Skip to content

Commit 88d0626

Browse files
jlhe97meta-codesync[bot]
authored andcommitted
folly/io_uring: deliver writeStarting() exactly once via shared WriteCallbackWithState
Summary: What: Deliver `writeStarting()` to the `WriteCallback` exactly once per write on the native `AsyncSocket` + io_uring send path. `IoUringSend`'s `SendRequest` now holds the `WriteCallbackWithState`, and `onSendStarted()` calls `notifyOnWrite()` (which dedups via `writeInProgress_`); `AsyncSocket::writeImpl` passes its `callbackWithState` into `iouSendHandle_->write()` so an immediate write arrives already-notified. Why: The io_uring send handle fired `writeStarting()` on every SQE submit while `AsyncSocket` also fired it via `notifyOnWrite()`, so a single write got two calls (and more on partial / event-re-registration re-submits). `RocketServerConnection::writeStarting()` DCHECKs that `startRawByteOffset` is unset, so the duplicate aborts the process (`RocketServerConnection.cpp:718`); in opt builds it silently corrupts the byte offset. This crashed graphstore once it served on native `AsyncSocket` over io_uring with zero-copy (D111176951). ``` Rocket flushWrites -> AsyncSocket::writeImpl |- notifyOnWrite() --------------> writeStarting() #1 (sets startRawByteOffset) `- iouSendHandle_->write -> submit `- onSendStarted() ---------> writeStarting() facebook#2 -> DCHECK abort (+ again on every partial / re-arm re-submit) fix: both paths funnel through one WriteCallbackWithState.notifyOnWrite() -> fires once ``` Rendered flow: https://pxl.cl/bBVG9 Reviewed By: spikeh Differential Revision: D111316678 fbshipit-source-id: 037ff58303d0fbff4e8e77af6b3921f9463a7196
1 parent 240225b commit 88d0626

4 files changed

Lines changed: 90 additions & 13 deletions

File tree

third-party/folly/src/folly/io/async/AsyncSocket.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2102,7 +2102,7 @@ void AsyncSocket::writeImpl(
21022102
}
21032103
}
21042104
iouSendHandle_->write(
2105-
callback,
2105+
callbackWithState,
21062106
vec + countWritten,
21072107
count - countWritten,
21082108
partialWritten,

third-party/folly/src/folly/io/async/IoUringSend.cpp

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class IoUringSendHandle::SendRequest : public IoSqeBase {
3939
}
4040

4141
explicit SendRequest(
42-
AsyncWriter::WriteCallback* callback,
42+
WriteCallbackWithState callback,
4343
const struct iovec* iov,
4444
size_t iovCount,
4545
size_t partialWritten,
@@ -48,8 +48,11 @@ class IoUringSendHandle::SendRequest : public IoSqeBase {
4848
WriteFlags flags,
4949
NetworkSocket fd)
5050
: IoSqeBase(IoSqeBase::Type::Write),
51-
callback_(callback),
52-
releaseCb_(callback ? callback->getReleaseIOBufCallback() : nullptr),
51+
callbackWithState_(callback),
52+
releaseCb_(
53+
callback.getCallback()
54+
? callback.getCallback()->getReleaseIOBufCallback()
55+
: nullptr),
5356
iovRemaining_(iovCount),
5457
bytesWritten_(bytesWritten),
5558
data_(std::move(data)),
@@ -81,7 +84,10 @@ class IoUringSendHandle::SendRequest : public IoSqeBase {
8184
}
8285
SendRequest* getNext() { return next_; }
8386
void append(SendRequest* request) { next_ = request; }
84-
AsyncWriter::WriteCallback* getCallback() { return callback_; }
87+
AsyncWriter::WriteCallback* getCallback() {
88+
return callbackWithState_.getCallback();
89+
}
90+
void notifyOnWrite() { callbackWithState_.notifyOnWrite(); }
8591
size_t getTotalBytesWritten() { return bytesWritten_; }
8692
folly::IOBuf* getData() const { return data_.get(); }
8793
bool notifPending() const { return refs_ > 1; }
@@ -107,7 +113,7 @@ class IoUringSendHandle::SendRequest : public IoSqeBase {
107113
CHECK(handle_ == nullptr);
108114
void* buf = alloc(msg_.msg_iovlen);
109115
auto clone = new (buf) SendRequest(
110-
callback_,
116+
callbackWithState_,
111117
msg_.msg_iov,
112118
msg_.msg_iovlen,
113119
0,
@@ -222,7 +228,7 @@ class IoUringSendHandle::SendRequest : public IoSqeBase {
222228
msg_.msg_iovlen = std::min<size_t>(iovRemaining_, kIovMax);
223229
}
224230

225-
AsyncWriter::WriteCallback* callback_;
231+
WriteCallbackWithState callbackWithState_;
226232
AsyncWriter::ReleaseIOBufCallback* releaseCb_;
227233
size_t iovRemaining_;
228234
size_t bytesWritten_;
@@ -338,7 +344,7 @@ bool IoUringSendHandle::update(uint16_t eventFlags) {
338344
}
339345

340346
void IoUringSendHandle::write(
341-
AsyncWriter::WriteCallback* callback,
347+
WriteCallbackWithState callback,
342348
const struct iovec* iov,
343349
size_t iovCount,
344350
size_t partialWritten,
@@ -412,9 +418,7 @@ void IoUringSendHandle::trySubmit() {
412418
}
413419

414420
void IoUringSendHandle::onSendStarted() {
415-
if (auto* cb = requestHead_->getCallback()) {
416-
cb->writeStarting();
417-
}
421+
requestHead_->notifyOnWrite();
418422
}
419423

420424
void IoUringSendHandle::onSendPartial(size_t bytesWritten) {
@@ -516,7 +520,7 @@ bool IoUringSendHandle::update(uint16_t /*eventFlags*/) {
516520
}
517521

518522
void IoUringSendHandle::write(
519-
AsyncWriter::WriteCallback* /*callback*/,
523+
WriteCallbackWithState /*callback*/,
520524
const struct iovec* /*iov*/,
521525
size_t /*iovCount*/,
522526
size_t /*partialWritten*/,

third-party/folly/src/folly/io/async/IoUringSend.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <folly/io/async/AsyncTransport.h>
2424
#include <folly/io/async/DelayedDestruction.h>
2525
#include <folly/io/async/IoUringBase.h>
26+
#include <folly/io/async/WriteCallbackWithState.h>
2627
#include <folly/net/NetworkSocket.h>
2728

2829
namespace folly {
@@ -59,7 +60,7 @@ class IoUringSendHandle : public DelayedDestruction {
5960

6061
bool update(uint16_t eventFlags);
6162
void write(
62-
AsyncWriter::WriteCallback* callback,
63+
WriteCallbackWithState callback,
6364
const struct iovec* iov,
6465
size_t iovCount,
6566
size_t partialWritten,

third-party/folly/src/folly/io/async/test/AsyncSocketTest2.cpp

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1127,6 +1127,78 @@ TEST_P(AsyncSocketTest, MoveEventBaseWithInflightZeroCopyWrite) {
11271127
server.verifyConnection(buf, kLen);
11281128
}
11291129

1130+
/**
1131+
* A partially-written buffer is handed to IoUringSendHandle and re-submitted to
1132+
* io_uring as the socket buffer drains. writeStarting() must reach the
1133+
* WriteCallback exactly once, not once per (re-)submission. Regression test for
1134+
* the double writeStarting() that aborted RocketServerConnection's DCHECK on
1135+
* the native AsyncSocket + io_uring send path.
1136+
*/
1137+
TEST_P(AsyncSocketTest, PartialWriteFiresWriteStartingOnce) {
1138+
if (GetParam() != BackendType::IO_URING) {
1139+
GTEST_SKIP() << "IoUringSendHandle send path is io_uring-only";
1140+
}
1141+
1142+
// Small socket buffers so a large write can't complete in one shot.
1143+
constexpr size_t kSockBufSize = 8 * 1024;
1144+
TestServer server(false, kSockBufSize);
1145+
1146+
SocketOptionMap options{
1147+
{{SOL_SOCKET, SO_SNDBUF}, int(kSockBufSize)},
1148+
{{SOL_SOCKET, SO_RCVBUF}, int(kSockBufSize)},
1149+
{{IPPROTO_TCP, TCP_NODELAY}, 1},
1150+
};
1151+
1152+
// The receiver drains on this thread; the sender gets its own EVB thread.
1153+
EventBase& senderEvb = getEventBase();
1154+
std::thread senderThread([&]() { senderEvb.loopForever(); });
1155+
1156+
ConnCallback ccb;
1157+
WriteCallback wcb;
1158+
std::shared_ptr<AsyncSocket> socket;
1159+
1160+
senderEvb.runInEventBaseThreadAndWait([&]() {
1161+
socket = AsyncSocket::newSocket(&senderEvb);
1162+
socket->connect(&ccb, server.getAddress(), 30, options);
1163+
});
1164+
1165+
std::shared_ptr<BlockingSocket> acceptedSocket = server.accept();
1166+
1167+
// Completion is signalled via an atomic set on the sender thread; wcb's
1168+
// non-atomic fields are only read after that thread is joined.
1169+
std::atomic<bool> writeDone{false};
1170+
wcb.successCallback = [&writeDone]() { writeDone = true; };
1171+
1172+
// Big enough to overflow the send+recv buffers, so the first write is partial
1173+
// and its remainder is re-submitted through IoUringSendHandle as we drain.
1174+
constexpr size_t kSendSize = 100 * 1024;
1175+
auto const sendBuf = std::vector<char>(kSendSize, 'a');
1176+
1177+
senderEvb.runInEventBaseThreadAndWait([&]() {
1178+
socket->write(&wcb, sendBuf.data(), kSendSize);
1179+
});
1180+
1181+
// Drain everything so the write ultimately succeeds.
1182+
std::vector<uint8_t> recvBuf(kSendSize);
1183+
auto bytesRead = acceptedSocket->readAll(recvBuf.data(), recvBuf.size());
1184+
ASSERT_EQ(kSendSize, bytesRead);
1185+
EXPECT_EQ(0, memcmp(recvBuf.data(), sendBuf.data(), bytesRead));
1186+
1187+
using clock = std::chrono::steady_clock;
1188+
auto const deadline = clock::now() + std::chrono::seconds(30);
1189+
while (!writeDone.load() && clock::now() < deadline) {
1190+
std::this_thread::yield();
1191+
}
1192+
1193+
senderEvb.terminateLoopSoon();
1194+
senderThread.join();
1195+
socket.reset();
1196+
1197+
EXPECT_EQ(STATE_SUCCEEDED, wcb.state);
1198+
// The core assertion: exactly one writeStarting despite multiple submits.
1199+
EXPECT_EQ(1, wcb.writeStartingInvocations);
1200+
}
1201+
11301202
/**
11311203
* Test calling close() immediately after connect()
11321204
*/

0 commit comments

Comments
 (0)