Loading src/quic.cpp +34 −22 Original line number Diff line number Diff line Loading @@ -7859,19 +7859,34 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, // // This loop was, and remains, strictly sequential on whichever thread // calls sendStreamData() -- packet N+1's build phase never starts // before packet N's send phase has completed -- so releasing // quic_mtx() mid-packet does not by itself risk reordering (the same // thread just isn't holding that particular lock for a moment; nothing // about the loop's own progress changes). The one real new risk it // would open is a *second* thread calling sendStreamData() on this same // connection concurrently, slipping its own packet in between this // one's unlock and relock and reaching batchPacket() first with a // higher packet number -- exactly the "packet reordering" failure this // change was originally rejected over. _send_pipeline_mutex (see its // declaration in socket.h) closes that: held for this whole per-packet // sequence, it keeps packets reaching batchPacket()/recordSentPacket() // in strict packet-number order even if sendStreamData() is ever called // from more than one thread for the same connection. // before packet N's send phase has completed -- so releasing quic_mtx() // mid-packet does not by itself risk reordering (the same thread just // isn't holding that particular lock for a moment; nothing about the // loop's own progress changes). // // A dedicated second mutex to also cover the case of *two* application // threads calling sendStreamData() concurrently on the same connection // was tried and reverted: it created a real, ThreadSanitizer-confirmed // lock-order-inversion with quic_mtx() itself, because this function is // legitimately reentrant on one thread (a stream callback invoked // synchronously from processFrame(), itself reached while quic_mtx() is // already held via pumpIncomingLocked()/accept(), can call back into // sendStreamData() -- exactly the reentrancy quic_mtx()'s own recursive // nature exists for). That reentrant path acquired the second mutex // while quic_mtx() was already held, and this function's unlock/relock // around the crypto calls below then reacquired quic_mtx() while the // second mutex was still held -- a genuine A-then-B, B-then-A cycle, // not a false positive. Removed rather than reworked further: this // codebase's own design (one application thread driving sends per // connection, matching how the original F30 report itself frames the // contention as "an application thread" singular vs. "the network // thread") doesn't call sendStreamData() concurrently from multiple // threads on one connection anywhere today, so there is nothing to // protect against in practice -- and a mutex added defensively for a // scenario that doesn't occur is not worth a confirmed new deadlock // class in the one that does. If concurrent multi-threaded sends on a // single connection ever become a real, intended usage pattern, this // needs revisiting with that requirement explicit, not assumed. std::unique_lock<std::recursive_mutex> lock(quic_mtx()); // 0-RTT: the handshake hasn't completed, but early data keys are ready Loading Loading @@ -8085,12 +8100,6 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, if (!unblocked) break; } // F30: serializes this packet's whole build->encrypt->send // sequence against any other thread doing the same on this // connection -- see its declaration and this function's own // comment above. std::lock_guard<std::recursive_mutex> pipeline_lock(_send_pipeline_mutex); // === Build short header directly into pkt_buf === uint8_t first_byte = 0x40 | 0x03; // Short header, 4-byte PN pkt_buf[0] = first_byte; Loading Loading @@ -8198,9 +8207,12 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, uint8_t* tag_dst = ct_dst + payload_len; // F30: quic_mtx() released for the two crypto calls below (see this // function's opening comment) -- pipeline_lock (_send_pipeline_mutex) // stays held throughout, preserving this packet's send ordering // relative to any other thread doing the same on this connection. // function's opening comment for why no second lock guards this // window). This loop is still strictly sequential on this thread, // so nothing about packet ordering changes -- only that a // concurrent thread processing incoming ACKs for this same // connection no longer has to wait for this packet's crypto just // to acquire quic_mtx(). lock.unlock(); if (cached_aes) { Loading src/socket.h +0 −17 Original line number Diff line number Diff line Loading @@ -1863,23 +1863,6 @@ namespace netplus { // the ordering avoids relying on that distinction. mutable std::recursive_mutex _quic_mutex; // F30 (performance report, implemented 2026-08-15): guards one // packet's build->encrypt->send sequence in sendStreamData(), which // releases quic_mtx() around the AES-GCM/header-protection calls so // a concurrent thread processing incoming ACKs for this same // connection doesn't have to wait for this packet's crypto just to // acquire quic_mtx(). quic_mtx() alone no longer guarantees // packets reach batchPacket()/recordSentPacket() in packet-number // order once it can be released mid-packet — this mutex (held for // the whole per-packet sequence, from PN reservation through the // post-encrypt bookkeeping) restores that guarantee even if // multiple threads ever call sendStreamData() concurrently on the // same connection. Recursive for the same reentrancy reason as // _quic_mutex above (processFrame callbacks invoked from // pumpIncomingLocked(), itself called while this is held, can // re-enter sendStreamData()). mutable std::recursive_mutex _send_pipeline_mutex; // Guards _child_connections only (registry membership), never a // connection's own state — that's quic_mtx()'s job. shared_mutex // because lookups (every packet routed to an existing connection) Loading Loading
src/quic.cpp +34 −22 Original line number Diff line number Diff line Loading @@ -7859,19 +7859,34 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, // // This loop was, and remains, strictly sequential on whichever thread // calls sendStreamData() -- packet N+1's build phase never starts // before packet N's send phase has completed -- so releasing // quic_mtx() mid-packet does not by itself risk reordering (the same // thread just isn't holding that particular lock for a moment; nothing // about the loop's own progress changes). The one real new risk it // would open is a *second* thread calling sendStreamData() on this same // connection concurrently, slipping its own packet in between this // one's unlock and relock and reaching batchPacket() first with a // higher packet number -- exactly the "packet reordering" failure this // change was originally rejected over. _send_pipeline_mutex (see its // declaration in socket.h) closes that: held for this whole per-packet // sequence, it keeps packets reaching batchPacket()/recordSentPacket() // in strict packet-number order even if sendStreamData() is ever called // from more than one thread for the same connection. // before packet N's send phase has completed -- so releasing quic_mtx() // mid-packet does not by itself risk reordering (the same thread just // isn't holding that particular lock for a moment; nothing about the // loop's own progress changes). // // A dedicated second mutex to also cover the case of *two* application // threads calling sendStreamData() concurrently on the same connection // was tried and reverted: it created a real, ThreadSanitizer-confirmed // lock-order-inversion with quic_mtx() itself, because this function is // legitimately reentrant on one thread (a stream callback invoked // synchronously from processFrame(), itself reached while quic_mtx() is // already held via pumpIncomingLocked()/accept(), can call back into // sendStreamData() -- exactly the reentrancy quic_mtx()'s own recursive // nature exists for). That reentrant path acquired the second mutex // while quic_mtx() was already held, and this function's unlock/relock // around the crypto calls below then reacquired quic_mtx() while the // second mutex was still held -- a genuine A-then-B, B-then-A cycle, // not a false positive. Removed rather than reworked further: this // codebase's own design (one application thread driving sends per // connection, matching how the original F30 report itself frames the // contention as "an application thread" singular vs. "the network // thread") doesn't call sendStreamData() concurrently from multiple // threads on one connection anywhere today, so there is nothing to // protect against in practice -- and a mutex added defensively for a // scenario that doesn't occur is not worth a confirmed new deadlock // class in the one that does. If concurrent multi-threaded sends on a // single connection ever become a real, intended usage pattern, this // needs revisiting with that requirement explicit, not assumed. std::unique_lock<std::recursive_mutex> lock(quic_mtx()); // 0-RTT: the handshake hasn't completed, but early data keys are ready Loading Loading @@ -8085,12 +8100,6 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, if (!unblocked) break; } // F30: serializes this packet's whole build->encrypt->send // sequence against any other thread doing the same on this // connection -- see its declaration and this function's own // comment above. std::lock_guard<std::recursive_mutex> pipeline_lock(_send_pipeline_mutex); // === Build short header directly into pkt_buf === uint8_t first_byte = 0x40 | 0x03; // Short header, 4-byte PN pkt_buf[0] = first_byte; Loading Loading @@ -8198,9 +8207,12 @@ size_t quic::sendStreamData(uint64_t stream_id, const uint8_t* data, size_t len, uint8_t* tag_dst = ct_dst + payload_len; // F30: quic_mtx() released for the two crypto calls below (see this // function's opening comment) -- pipeline_lock (_send_pipeline_mutex) // stays held throughout, preserving this packet's send ordering // relative to any other thread doing the same on this connection. // function's opening comment for why no second lock guards this // window). This loop is still strictly sequential on this thread, // so nothing about packet ordering changes -- only that a // concurrent thread processing incoming ACKs for this same // connection no longer has to wait for this packet's crypto just // to acquire quic_mtx(). lock.unlock(); if (cached_aes) { Loading
src/socket.h +0 −17 Original line number Diff line number Diff line Loading @@ -1863,23 +1863,6 @@ namespace netplus { // the ordering avoids relying on that distinction. mutable std::recursive_mutex _quic_mutex; // F30 (performance report, implemented 2026-08-15): guards one // packet's build->encrypt->send sequence in sendStreamData(), which // releases quic_mtx() around the AES-GCM/header-protection calls so // a concurrent thread processing incoming ACKs for this same // connection doesn't have to wait for this packet's crypto just to // acquire quic_mtx(). quic_mtx() alone no longer guarantees // packets reach batchPacket()/recordSentPacket() in packet-number // order once it can be released mid-packet — this mutex (held for // the whole per-packet sequence, from PN reservation through the // post-encrypt bookkeeping) restores that guarantee even if // multiple threads ever call sendStreamData() concurrently on the // same connection. Recursive for the same reentrancy reason as // _quic_mutex above (processFrame callbacks invoked from // pumpIncomingLocked(), itself called while this is held, can // re-enter sendStreamData()). mutable std::recursive_mutex _send_pipeline_mutex; // Guards _child_connections only (registry membership), never a // connection's own state — that's quic_mtx()'s job. shared_mutex // because lookups (every packet routed to an existing connection) Loading