From e4b2d5fa083aead2f161c08b65fa7bae7aab01b0 Mon Sep 17 00:00:00 2001 From: Matt Millett Date: Tue, 1 Sep 2026 12:47:58 -0400 Subject: [PATCH 1/2] Interim checkpoint: use separate send/receive mutexes in stream sockets Both ntcr::StreamSocket (reactor-based) and ntcp::StreamSocket (proactor-based) previously serialized all operations on a single mutex, so a call to 'send' or the processing of a socket writable/sent event blocked a concurrent call to 'receive' or the processing of a socket readable/received event, and vice-versa. Introduce a distinct mutex for each direction so that the steady-state send and receive paths proceed independently. Locking strategy ---------------- Each stream socket now defines three mutexes, always acquired in the canonical order 'd_mutex' (state), then 'd_sendMutex', then 'd_receiveMutex': * 'd_sendMutex' guards the send queue, the send rate limiter and deflater, the send deadline and rate timers, and the send-side portion of an I/O completion. * 'd_receiveMutex' guards the receive queue, the receive rate limiter and inflater, the receive deadline and rate timers, and the receive-side portion of an I/O completion. * 'd_mutex' guards the socket lifecycle: opening, connecting, upgrading, downgrading, shutting down, closing, detaching, and the registration of managers, sessions, and resolvers. Steady-state I/O holds only its own leaf mutex. When a completion discovers a cross-cutting transition (end-of-stream, failure, or the graceful shutdown sentinel), the drain functions now return that condition to their caller rather than invoking the transition inline; the caller releases the leaf mutex and re-acquires all three mutexes in canonical order before performing the transition. Rare failures that cannot conveniently return a status are deferred onto the reactor or proactor via 'execute', so that they re-enter with no locks held. Members that are read from more than one direction -- 'd_socket_sp', 'd_encryption_sp', 'd_session_sp', 'd_sessionStrand_sp', 'd_manager_sp', and 'd_managerStrand_sp' -- are written only while holding all three mutexes and may be read while holding any one of them. Announcements and callback dispatches now name the mutex actually held at the point of dispatch, so that a synchronous dispatch unlocks and relocks the correct mutex. Any dispatch performed while holding more than one mutex is forced to defer. Encrypted sockets ----------------- A TLS receive completion may need to write outgoing ciphertext, which touches send-side state. The unencrypted path, which is the common case, therefore takes only its own leaf mutex, while the encrypted path checks for the presence of an encryption session under the leaf mutex, then re-acquires all three mutexes in canonical order and repeats the completion. Likewise, 'send' escalates to the state and send mutexes only when the socket is encrypted. Behavior preserved ------------------ Starting a new write still fails if the socket is not yet connected, is shut down in the send direction, or is being shut down in the send direction, while writes already queued still drain when the shutdown is graceful. Starting a new read still fails once the socket is shut down for reading and the receive queue has been fully dequeued. Supporting components --------------------- Lifecycle predicates must remain observable from a thread that holds only a leaf mutex, so the state components consulted on the I/O paths no longer require the state mutex to be read: * ntcs::OpenState stores its value in a 'bsls::AtomicInt'. * ntcs::DetachState stores its mode in a 'bsls::AtomicInt'. * ntcs::ShutdownState maintains atomic mirrors of the predicates derived from its context, refreshed whenever the context changes. * ntcs::FlowControlState is now thread safe. Its five boolean data members are packed into the bits of a single 'bsls::AtomicUint', and 'apply' and 'relax' commit their compound transitions with a compare-and-swap retry loop. This allows a single flow control state object to remain shared between the send and receive directions rather than being split per-direction. Tests ----- Update the ntcr::StreamSocket test driver so that its reactor mock expectations account for the cross-cutting announcements that are now delivered deferred: connect completion and failure, upgrade completion and failure, and error. The affected cases capture the functor passed to 'execute' and invoke it before asserting. The ntcp::StreamSocket test driver required no changes, as it exercises real sockets rather than mocks. --- groups/ntc/ntcp/ntcp_streamsocket.cpp | 564 ++++++++++++------ groups/ntc/ntcp/ntcp_streamsocket.h | 69 ++- groups/ntc/ntcr/ntcr_streamsocket.cpp | 686 ++++++++++++++++------ groups/ntc/ntcr/ntcr_streamsocket.h | 75 ++- groups/ntc/ntcr/ntcr_streamsocket.t.cpp | 77 +++ groups/ntc/ntcs/ntcs_detachstate.h | 11 +- groups/ntc/ntcs/ntcs_flowcontrolstate.cpp | 171 +++--- groups/ntc/ntcs/ntcs_flowcontrolstate.h | 33 +- groups/ntc/ntcs/ntcs_openstate.cpp | 2 +- groups/ntc/ntcs/ntcs_openstate.h | 30 +- groups/ntc/ntcs/ntcs_shutdownstate.cpp | 20 + groups/ntc/ntcs/ntcs_shutdownstate.h | 25 +- 12 files changed, 1278 insertions(+), 485 deletions(-) diff --git a/groups/ntc/ntcp/ntcp_streamsocket.cpp b/groups/ntc/ntcp/ntcp_streamsocket.cpp index b5a5c29a..45bd04ae 100644 --- a/groups/ntc/ntcp/ntcp_streamsocket.cpp +++ b/groups/ntc/ntcp/ntcp_streamsocket.cpp @@ -215,7 +215,11 @@ void StreamSocket::processSocketConnected(const ntsa::Error& error) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Connection completion is cross-cutting; acquire all three mutexes in + // canonical order. + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -245,35 +249,131 @@ void StreamSocket::processSocketReceived(const ntsa::Error& error, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); - if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { return; } - NTCI_LOG_CONTEXT(); + bool encrypted = false; + ntsa::Error outcome; - NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); - NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); - NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + // Fast path: an unencrypted receive completion only needs the receive + // mutex. The encryption object is co-guarded and may be examined under the + // receive mutex; if it is present, escalate to holding all three mutexes in + // canonical order (the encrypted completion decrypts under 'd_mutex' and may + // enqueue outgoing cipher text under 'd_sendMutex'). To avoid a concurrent + // 'privateInitiateReceive' from overwriting 'd_receiveBlob_sp' during the + // lock handoff, 'd_receivePending' is left true until the completion is + // actually processed. - d_receivePending = false; + { + LockGuard receiveLock(&d_receiveMutex); - if (error) { - if (error != ntsa::Error::e_CANCELLED) { - NTCP_STREAMSOCKET_LOG_RECEIVE_FAILURE(error); - this->privateFailReceive(self, error); + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; + } + + if (d_encryption_sp) { + encrypted = true; + } + else { + NTCI_LOG_CONTEXT(); + NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); + NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); + NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + + d_receivePending = false; + + if (error) { + if (error != ntsa::Error::e_CANCELLED) { + NTCP_STREAMSOCKET_LOG_RECEIVE_FAILURE(error); + outcome = error; + } + } + else { + NTCP_STREAMSOCKET_LOG_RECEIVE_RESULT(context); + outcome = this->privateCompleteReceive( + self, + context.bytesReceivable(), + context.bytesReceived(), + false); + } + + if (!outcome) { + this->privateInitiateReceive(self); + } } } - else { - NTCP_STREAMSOCKET_LOG_RECEIVE_RESULT(context); - this->privateCompleteReceive(self, - context.bytesReceivable(), - context.bytesReceived()); + + if (encrypted) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; + } + + NTCI_LOG_CONTEXT(); + NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); + NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); + NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + + d_receivePending = false; + + if (error) { + if (error != ntsa::Error::e_CANCELLED) { + NTCP_STREAMSOCKET_LOG_RECEIVE_FAILURE(error); + outcome = error; + } + } + else { + NTCP_STREAMSOCKET_LOG_RECEIVE_RESULT(context); + outcome = this->privateCompleteReceive(self, + context.bytesReceivable(), + context.bytesReceived(), + true); + } + + if (outcome == ntsa::Error::e_EOF) { + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); + } + else if (outcome) { + this->privateFailReceive(self, outcome); + } + else { + this->privateInitiateReceive(self); + } + + return; } - this->privateInitiateReceive(self); + // Perform any cross-cutting shutdown or failure sequence for the + // unencrypted path outside the receive mutex, acquiring all three mutexes + // in canonical order. + + if (outcome == ntsa::Error::e_EOF) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); + } + else if (outcome) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + this->privateFailReceive(self, outcome); + } } void StreamSocket::processSocketSent(const ntsa::Error& error, @@ -283,7 +383,7 @@ void StreamSocket::processSocketSent(const ntsa::Error& error, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -318,7 +418,11 @@ void StreamSocket::processSocketError(const ntsa::Error& error) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Error handling is cross-cutting (it fails the whole socket); acquire all + // three mutexes in canonical order. + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -338,7 +442,12 @@ void StreamSocket::processSocketDetached() { NTCCFG_OBJECT_GUARD(&d_object); - LockGuard lock(&d_mutex); + // The detachment completion runs the deferred shutdown/connect-failure + // completion, which is cross-cutting; acquire all three mutexes in + // canonical order. + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -364,7 +473,9 @@ void StreamSocket::processConnectDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -406,7 +517,9 @@ void StreamSocket::processConnectRetryTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -444,7 +557,9 @@ void StreamSocket::processUpgradeTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -473,7 +588,7 @@ void StreamSocket::processSendRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -502,7 +617,7 @@ void StreamSocket::processSendRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_sendMutex); } } } @@ -518,7 +633,7 @@ void StreamSocket::processSendDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -552,7 +667,7 @@ void StreamSocket::processSendDeadlineTimer( d_proactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } } @@ -567,7 +682,7 @@ void StreamSocket::processReceiveRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -596,7 +711,7 @@ void StreamSocket::processReceiveRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -612,7 +727,7 @@ void StreamSocket::processReceiveDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -640,7 +755,7 @@ void StreamSocket::processReceiveDeadlineTimer( d_proactorStrand_sp, self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -650,8 +765,10 @@ void StreamSocket::privateEncryptionHandshake( const bsl::shared_ptr& certificate, const bsl::string& details) { - // IMPLEMENTATION NOTE: This function is always called under a lock - // during the exection of 'd_encryption->popIncomingCipherText()'. + // IMPLEMENTATION NOTE: This function is always called while holding + // 'd_mutex', 'd_sendMutex', and 'd_receiveMutex' (from the encrypted + // receive completion or from 'privateUpgrade'), so the callback is + // deferred. NTCI_LOG_CONTEXT(); @@ -681,7 +798,7 @@ void StreamSocket::privateEncryptionHandshake( event, d_proactorStrand_sp, self, - false, + true, &d_mutex); } } @@ -833,6 +950,9 @@ void StreamSocket::privateCompleteConnect( NTCI_LOG_TRACE("Connection attempt succeeded"); + // Connection establishment runs while holding all three mutexes, so all + // announcements and callbacks are deferred. + if (d_session_sp) { ntcs::Dispatch::announceConnectComplete(d_session_sp, self, @@ -840,7 +960,7 @@ void StreamSocket::privateCompleteConnect( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, + true, &d_mutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { @@ -853,7 +973,7 @@ void StreamSocket::privateCompleteConnect( connectEvent, d_proactorStrand_sp, self, - false, + true, &d_mutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { @@ -866,7 +986,7 @@ void StreamSocket::privateCompleteConnect( d_managerStrand_sp, d_proactorStrand_sp, self, - false, + true, &d_mutex); } @@ -1013,6 +1133,12 @@ void StreamSocket::privateFailConnectComplete( bool defer) { NTCI_LOG_CONTEXT(); + + // This cleanup always runs while holding all three mutexes (inline from + // 'privateFailConnect' or from the deferred detachment completion), so all + // announcements and callbacks are deferred. + NTCCFG_WARNING_UNUSED(defer); + if (d_systemHandle != ntsa::k_INVALID_HANDLE) { if (d_socket_sp) { ntcs::ObserverRef proactorRef(&d_proactor); @@ -1045,7 +1171,7 @@ void StreamSocket::privateFailConnectComplete( d_sessionStrand_sp, ntci::Strand::unknown(), self, - defer, + true, &d_mutex); } @@ -1054,7 +1180,7 @@ void StreamSocket::privateFailConnectComplete( connectEvent, d_proactorStrand_sp, self, - defer, + true, &d_mutex); } @@ -1119,7 +1245,7 @@ void StreamSocket::privateFailUpgrade( this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1129,14 +1255,14 @@ void StreamSocket::privateFailUpgrade( upgradeEvent, d_proactorStrand_sp, self, - false, + true, &d_mutex); } this->privateShutdown(self, ntsa::ShutdownType::e_BOTH, ntsa::ShutdownMode::e_IMMEDIATE, - false); + true); } void StreamSocket::privateInitiateReceive( @@ -1158,7 +1284,12 @@ void StreamSocket::privateInitiateReceive( ntcs::ObserverRef proactorRef(&d_proactor); if (!proactorRef) { - this->privateFailReceive(self, ntsa::Error(ntsa::Error::e_INVALID)); + // Failing the receive is cross-cutting; this function runs holding only + // the receive mutex, so defer the failure to run under all three + // mutexes in canonical order. + this->execute(NTCCFG_BIND(&StreamSocket::processSocketError, + self, + ntsa::Error(ntsa::Error::e_INVALID))); return; } @@ -1179,51 +1310,31 @@ void StreamSocket::privateInitiateReceive( error = proactorRef->receive(self, d_receiveBlob_sp.get(), d_receiveOptions); if (error) { - this->privateFailReceive(self, error); + // Failing the receive is cross-cutting; defer it to run under all three + // mutexes in canonical order. + this->execute( + NTCCFG_BIND(&StreamSocket::processSocketError, self, error)); return; } d_receivePending = true; } -void StreamSocket::privateCompleteReceive( +ntsa::Error StreamSocket::privateCompleteReceive( const bsl::shared_ptr& self, bsl::size_t numBytesReceivable, - bsl::size_t numBytesReceived) + bsl::size_t numBytesReceived, + bool defer) { NTCI_LOG_CONTEXT(); ntsa::Error error; if (numBytesReceived == 0) { - // Execute the shutdown of the receive buffer asynchronously to allow - // pending writes to first complete, otherwise pending writes which - // have had their data copied to the socket send buffer but not - // announced to be complete by the proactor will be mistakenly - // considered by be discarded. - // - // However, there is no current API to call something like - // 'privateShutdownReceive' indicating the peer has initiated the - // shutdown while not expected that function to be holding the lock. - // - // TODO: Resolve this issue. - // - // this->privateApplyFlowControl(self, - // ntca::FlowControlType::e_RECEIVE, - // ntca::FlowControlMode::e_IMMEDIATE, - // true); - // - // this->execute(NTCCFG_BIND( - // &StreamSocket::privateShutdownReceive, - // self, - // self, - // ntsa::ShutdownOrigin::e_REMOTE, - // false)); - - this->privateShutdownReceive(self, - ntsa::ShutdownOrigin::e_REMOTE, - false); - return; + // The peer has shut down. Return EOF so the caller + // ('processSocketReceived') can perform the cross-cutting shutdown of + // reception after releasing the receive mutex. + return ntsa::Error(ntsa::Error::e_EOF); } d_receiveFeedback.setFeedback(numBytesReceivable, numBytesReceived); @@ -1257,8 +1368,7 @@ void StreamSocket::privateCompleteReceive( *d_receiveBlob_sp, inflateOptions); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } bdlbb::BlobUtil::erase(d_receiveBlob_sp.get(), @@ -1274,8 +1384,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->pushIncomingCipherText(*d_receiveBlob_sp); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } bdlbb::BlobUtil::erase(d_receiveBlob_sp.get(), @@ -1283,7 +1392,7 @@ void StreamSocket::privateCompleteReceive( d_receiveBlob_sp->length()); if (!d_encryption_sp) { - return; + return ntsa::Error(); } if (!d_receiveInflater_sp) { @@ -1291,8 +1400,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->popIncomingPlainText( d_receiveQueue.data().get()); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } } @@ -1306,7 +1414,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->popIncomingLeftovers( d_receiveQueue.data().get()); if (NTCCFG_UNLIKELY(error)) { - return; + return ntsa::Error(); } } } @@ -1316,8 +1424,7 @@ void StreamSocket::privateCompleteReceive( while (NTCCFG_LIKELY(d_encryption_sp->hasIncomingPlainText())) { error = d_encryption_sp->popIncomingPlainText(&plainText); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } } @@ -1330,7 +1437,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->popIncomingLeftovers(&plainText); if (NTCCFG_UNLIKELY(error)) { - return; + return ntsa::Error(); } } } @@ -1345,8 +1452,7 @@ void StreamSocket::privateCompleteReceive( plainText, inflateOptions); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } bdlbb::BlobUtil::append(d_receiveQueue.data().get(), inflatedData); @@ -1448,8 +1554,8 @@ void StreamSocket::privateCompleteReceive( receiveEvent, d_proactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } if (d_receiveQueue.authorizeLowWatermarkEvent()) { @@ -1468,8 +1574,8 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } @@ -1481,7 +1587,7 @@ void StreamSocket::privateCompleteReceive( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + defer, false); if (d_session_sp) { @@ -1495,8 +1601,8 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } @@ -1517,7 +1623,7 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, + defer, &d_mutex); } } @@ -1537,7 +1643,7 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, + defer, &d_mutex); } } @@ -1546,8 +1652,7 @@ void StreamSocket::privateCompleteReceive( if (!d_encryption_sp->isShutdownSent()) { error = d_encryption_sp->shutdown(); if (error) { - this->privateFailReceive(self, error); - return; + return error; } if (d_session_sp) { @@ -1564,7 +1669,7 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, + defer, &d_mutex); } } @@ -1579,8 +1684,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->popOutgoingCipherText(&cipherData); if (error) { - this->privateFailReceive(self, error); - return; + return error; } } @@ -1591,8 +1695,7 @@ void StreamSocket::privateCompleteReceive( ntca::SendContext(), d_sendComplete); if (error) { - this->privateFailReceive(self, error); - return; + return error; } } } @@ -1614,12 +1717,14 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, + defer, &d_mutex); } } } } + + return ntsa::Error(); } void StreamSocket::privateFailReceive( @@ -1710,8 +1815,16 @@ void StreamSocket::privateInitiateSend( break; } else { + // A graceful shutdown sentinel has been drained. Shutting down for + // sending is cross-cutting; this function runs holding only the + // send mutex, so defer the shutdown to run under all three mutexes + // in canonical order (via the public 'shutdown', which is a no-op + // sentinel push once the write queue is empty). d_sendQueue.popEntry(); - this->privateShutdownSend(self, false); + this->execute(NTCCFG_BIND(&StreamSocket::shutdown, + self, + ntsa::ShutdownType::e_SEND, + ntsa::ShutdownMode::e_GRACEFUL)); } } } @@ -1760,7 +1873,7 @@ void StreamSocket::privateCompleteSend( d_proactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } if (d_sendQueue.authorizeLowWatermarkEvent()) { @@ -1780,7 +1893,7 @@ void StreamSocket::privateCompleteSend( d_proactorStrand_sp, self, true, - &d_mutex); + &d_sendMutex); } } @@ -1817,7 +1930,7 @@ void StreamSocket::privateFailSend(const bsl::shared_ptr& self, d_proactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } @@ -1837,7 +1950,7 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1879,7 +1992,7 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, d_sessionStrand_sp, d_proactorStrand_sp, self, - false, + true, &d_mutex); } @@ -2433,7 +2546,7 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } this->privateInitiateSend(self); @@ -2458,7 +2571,7 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } this->privateInitiateReceive(self); @@ -2513,7 +2626,7 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } // Handled at the completion of the last send. @@ -2536,7 +2649,7 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } // Handled at the completion of the last receive. @@ -2575,7 +2688,7 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } @@ -2596,7 +2709,7 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2637,7 +2750,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_SEND, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canSend()) { @@ -2705,7 +2818,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canReceive()) { @@ -2924,14 +3037,14 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - const bool defer = !options.recurse(); - + // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so + // the completion callback is always deferred. callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - defer, - &d_mutex); + true, + &d_sendMutex); } return ntsa::Error(); @@ -2972,14 +3085,14 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - const bool defer = !options.recurse(); - + // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so + // the completion callback is always deferred. callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - defer, - &d_mutex); + true, + &d_sendMutex); } return ntsa::Error(); @@ -3367,7 +3480,9 @@ void StreamSocket::processRemoteEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -3469,7 +3584,7 @@ void StreamSocket::processRemoteEndpointResolution( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, + true, &d_mutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == @@ -3973,7 +4088,7 @@ ntsa::Error StreamSocket::privateRetryConnectToEndpoint( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, + true, &d_mutex); } @@ -4026,6 +4141,8 @@ StreamSocket::StreamSocket( bslma::Allocator* basicAllocator) : d_object("ntcp::StreamSocket") , d_mutex() +, d_sendMutex() +, d_receiveMutex() , d_transport(ntsa::Transport::e_UNDEFINED) , d_systemHandle(ntsa::k_INVALID_HANDLE) , d_systemSourceEndpoint() @@ -4656,7 +4773,9 @@ ntsa::Error StreamSocket::upgrade( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -4814,8 +4933,29 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Fast path: an unencrypted send only needs the send mutex. The encryption + // object is co-guarded and may be examined under the send mutex; if it is + // present, escalate to also holding the state mutex (in canonical order), + // since the encrypted send path operates on the shared encryption object. + + { + LockGuard sendLock(&d_sendMutex); + if (NTCCFG_LIKELY(!d_encryption_sp)) { + return this->privateSend(self, data, options, callback); + } + } + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + return this->privateSend(self, data, options, callback); +} + +ntsa::Error StreamSocket::privateSend( + const bsl::shared_ptr& self, + const bdlbb::Blob& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback) +{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -4854,7 +4994,7 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -4934,8 +5074,27 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Fast path: an unencrypted send only needs the send mutex; escalate to + // also hold the state mutex (in canonical order) only when encrypted. + { + LockGuard sendLock(&d_sendMutex); + if (NTCCFG_LIKELY(!d_encryption_sp)) { + return this->privateSend(self, data, options, callback); + } + } + + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + return this->privateSend(self, data, options, callback); +} + +ntsa::Error StreamSocket::privateSend( + const bsl::shared_ptr& self, + const ntsa::Data& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback) +{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -4974,7 +5133,7 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -5045,7 +5204,7 @@ ntsa::Error StreamSocket::receive(ntca::ReceiveContext* context, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5167,7 +5326,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5269,7 +5428,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); bool receiveQueueHighWatermarkViolatedAfter = d_receiveQueue.isHighWatermarkViolated(); @@ -5348,7 +5507,9 @@ ntsa::Error StreamSocket::registerManager( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (manager) { d_manager_sp = manager; @@ -5368,7 +5529,9 @@ ntsa::Error StreamSocket::registerManager( ntsa::Error StreamSocket::deregisterManager() { - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); d_manager_sp.reset(); d_managerStrand_sp.reset(); @@ -5381,7 +5544,9 @@ ntsa::Error StreamSocket::registerSession( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (session) { d_session_sp = session; @@ -5411,7 +5576,9 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (callback) { bsl::shared_ptr session; @@ -5448,7 +5615,9 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (callback) { bsl::shared_ptr session; @@ -5478,7 +5647,9 @@ ntsa::Error StreamSocket::registerSessionCallback( ntsa::Error StreamSocket::deregisterSession() { - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); d_session_sp.reset(); d_sessionStrand_sp.reset(); @@ -5497,7 +5668,7 @@ ntsa::Error StreamSocket::setConnectRateLimiter( ntsa::Error StreamSocket::setWriteDeflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); d_sendDeflater_sp = compression; @@ -5509,7 +5680,7 @@ ntsa::Error StreamSocket::setWriteRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5538,7 +5709,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5577,7 +5748,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5617,7 +5788,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5678,7 +5849,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntsa::Error StreamSocket::setReadInflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_receiveInflater_sp = compression; @@ -5690,7 +5861,7 @@ ntsa::Error StreamSocket::setReadRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5719,7 +5890,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5760,7 +5931,7 @@ ntsa::Error StreamSocket::setReadQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5786,7 +5957,7 @@ ntsa::Error StreamSocket::setReadQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5820,7 +5991,9 @@ ntsa::Error StreamSocket::relaxFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5837,7 +6010,9 @@ ntsa::Error StreamSocket::applyFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5879,7 +6054,9 @@ ntsa::Error StreamSocket::cancel(const ntca::ConnectToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5905,7 +6082,9 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5954,7 +6133,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6000,7 +6179,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6041,7 +6220,9 @@ ntsa::Error StreamSocket::downgrade() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); return this->privateDowngrade(self, ntca::DowngradeOptions()); } @@ -6050,7 +6231,9 @@ ntsa::Error StreamSocket::downgrade(const ntca::DowngradeOptions& options) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (options.abortive().isNull() || !options.abortive().value()) { return this->privateDowngrade(self, options); @@ -6065,7 +6248,9 @@ ntsa::Error StreamSocket::shutdown(ntsa::ShutdownType::Value direction, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6114,7 +6299,9 @@ ntsa::Error StreamSocket::release(ntsa::Handle* result, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); *result = ntsa::k_INVALID_HANDLE; @@ -6150,7 +6337,9 @@ void StreamSocket::close(const ntci::CloseCallback& callback) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); this->privateClose(self, callback); } @@ -6353,37 +6542,37 @@ bsl::size_t StreamSocket::threadIndex() const bsl::size_t StreamSocket::readQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.size(); } bsl::size_t StreamSocket::readQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.lowWatermark(); } bsl::size_t StreamSocket::readQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.highWatermark(); } bsl::size_t StreamSocket::writeQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.size(); } bsl::size_t StreamSocket::writeQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.lowWatermark(); } bsl::size_t StreamSocket::writeQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.highWatermark(); } @@ -6439,23 +6628,28 @@ void StreamSocket::getInfo(ntsa::SocketInfo* result) const d_publicRemoteEndpoint.load(&remoteEndpoint); { - LockGuard lock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + sendQueueSize = d_sendQueue.size(); + } - sendQueueSize = d_sendQueue.size(); + { + LockGuard receiveLock(&d_receiveMutex); receiveQueueSize = d_receiveQueue.size(); + } - if (d_shutdownState.completed()) { - socketState = ntsa::SocketState::e_CLOSED; - } - else if (d_shutdownState.initiated()) { - socketState = ntsa::SocketState::e_CLOSING; - } - else if (d_connectInProgress) { - socketState = ntsa::SocketState::e_SYN_SENT; - } - else { - socketState = ntsa::SocketState::e_ESTABLISHED; - } + // The shutdown and connect predicates are atomic and read without a lock. + + if (d_shutdownState.completed()) { + socketState = ntsa::SocketState::e_CLOSED; + } + else if (d_shutdownState.initiated()) { + socketState = ntsa::SocketState::e_CLOSING; + } + else if (d_connectInProgress) { + socketState = ntsa::SocketState::e_SYN_SENT; + } + else { + socketState = ntsa::SocketState::e_ESTABLISHED; } ntcs::ObserverRef proactorRef(&d_proactor); diff --git a/groups/ntc/ntcp/ntcp_streamsocket.h b/groups/ntc/ntcp/ntcp_streamsocket.h index b585f6d1..ab5221e4 100644 --- a/groups/ntc/ntcp/ntcp_streamsocket.h +++ b/groups/ntc/ntcp/ntcp_streamsocket.h @@ -88,7 +88,34 @@ class StreamSocket : public ntci::StreamSocket, typedef ntccfg::LockGuard LockGuard; ntccfg::Object d_object; + // LOCKING STRATEGY + // + // 'd_mutex' (the "state" mutex) guards the shared lifecycle and control + // state of the socket: the connect/upgrade machinery, the open, shutdown, + // and detach state machines' *transitions*, and the encryption object. + // + // 'd_sendMutex' guards the write path (the send queue and outgoing + // completion state) and 'd_receiveMutex' guards the read path (the receive + // queue and incoming completion state). These two leaf mutexes are + // independent so that sending is not serialized against receiving. + // + // When more than one is held they are always acquired in the order + // 'd_mutex' -> 'd_sendMutex' -> 'd_receiveMutex'. A steady-state I/O + // completion holds only its own leaf; when it must perform a cross-cutting + // lifecycle transition (connection establishment, failure, shutdown) it + // first releases its leaf, then re-acquires in canonical order (or defers + // the transition via 'execute'). + // + // The observable lifecycle predicates ('d_openState', 'd_shutdownState', + // 'd_detachState.mode()', 'd_connectInProgress') are atomic and may be read + // without any lock. 'd_flowControlState' is itself thread safe (lock-free). + // The co-guarded members ('d_socket_sp', 'd_encryption_sp', 'd_session_sp', + // 'd_sessionStrand_sp', 'd_manager_sp', 'd_managerStrand_sp') are written + // only while holding all three mutexes and may therefore be read while + // holding any one of them. mutable Mutex d_mutex; + mutable Mutex d_sendMutex; + mutable Mutex d_receiveMutex; ntsa::Transport::Value d_transport; ntsa::Handle d_systemHandle; ntsa::Endpoint d_systemSourceEndpoint; @@ -144,7 +171,7 @@ class StreamSocket : public ntci::StreamSocket, bsl::shared_ptr d_connectRetryTimer_sp; bsl::shared_ptr d_connectRateLimiter_sp; bsl::shared_ptr d_connectRateTimer_sp; - bool d_connectInProgress; + bsls::AtomicBool d_connectInProgress; ntca::UpgradeOptions d_upgradeOptions; ntci::UpgradeCallback d_upgradeCallback; bsl::shared_ptr d_upgradeTimer_sp; @@ -267,13 +294,21 @@ class StreamSocket : public ntci::StreamSocket, /// Initiate a new reception, if allowed and necessary. void privateInitiateReceive(const bsl::shared_ptr& self); - /// Process the completion of the reception of raw or encrypted - /// data according to the specified 'numBytesReceivable' and - /// 'numBytesReceived'. The behavior is undefined unless 'd_mutex' is - /// locked. - void privateCompleteReceive(const bsl::shared_ptr& self, - bsl::size_t numBytesReceivable, - bsl::size_t numBytesReceived); + /// Process the completion of the reception of raw or encrypted data + /// according to the specified 'numBytesReceivable' and 'numBytesReceived'. + /// If the specified 'defer' flag is true, force all event announcements and + /// callback invocations made during processing to be deferred (used when + /// running while holding more than one mutex, e.g. the encrypted read + /// path). Return 'ntsa::Error::e_EOF' if the peer has shut down, another + /// error if the reception failed, or a default-constructed error on + /// success; the caller performs any resulting shutdown or failure sequence + /// after releasing the receive mutex. The behavior is undefined unless + /// 'd_receiveMutex' is locked. + ntsa::Error privateCompleteReceive( + const bsl::shared_ptr& self, + bsl::size_t numBytesReceivable, + bsl::size_t numBytesReceived, + bool defer); /// Process the completion of the reception of the raw or unencrypted /// data according to the specified 'numBytesReceived'. The behavior is @@ -394,6 +429,24 @@ class StreamSocket : public ntci::StreamSocket, ntsa::Error privateThrottleReceiveBuffer( const bsl::shared_ptr& self); + /// Enqueue the specified 'data' for transmission according to the + /// specified 'options', invoking the specified 'callback' on completion. + /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the + /// socket is encrypted, 'd_mutex' as well). Return the error. + ntsa::Error privateSend(const bsl::shared_ptr& self, + const bdlbb::Blob& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback); + + /// Enqueue the specified 'data' for transmission according to the + /// specified 'options', invoking the specified 'callback' on completion. + /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the + /// socket is encrypted, 'd_mutex' as well). Return the error. + ntsa::Error privateSend(const bsl::shared_ptr& self, + const ntsa::Data& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback); + /// Send the specified raw or already encrypted 'data' according to the /// specified 'options'. When the 'data' is entirely copied to the /// send buffer, invoke the specified 'callback' on callback's strand. diff --git a/groups/ntc/ntcr/ntcr_streamsocket.cpp b/groups/ntc/ntcr/ntcr_streamsocket.cpp index 8e8f9b79..7efb91b3 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.cpp +++ b/groups/ntc/ntcr/ntcr_streamsocket.cpp @@ -288,14 +288,14 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); - NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + // These lifecycle predicates are atomic and may be read without any lock. + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { return; @@ -306,34 +306,135 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) } ntsa::Error error; - bsl::size_t numIterations = 0; + bool encrypted = false; - while (true) { - ++numIterations; + // Fast path: an unencrypted read only needs the receive mutex. The + // encryption object is co-guarded and may be examined while holding the + // receive mutex; if it is present, escalate to holding all three mutexes in + // canonical order (the encrypted read path decrypts under 'd_mutex' and may + // enqueue outgoing cipher text under 'd_sendMutex'). - error = this->privateSocketReadableIteration(self); - if (error) { - break; - } + { + LockGuard receiveLock(&d_receiveMutex); - if (!d_receiveGreedily) { - break; + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; } if (!d_shutdownState.canReceive()) { - break; + return; + } + + if (d_encryption_sp) { + encrypted = true; + } + else { + bsl::size_t numIterations = 0; + + while (true) { + ++numIterations; + + error = this->privateSocketReadableIteration(self, false); + if (error) { + break; + } + + if (!d_receiveGreedily) { + break; + } + + if (!d_shutdownState.canReceive()) { + break; + } + } + + if (numIterations > 0) { + NTCS_METRICS_UPDATE_RECEIVE_ITERATIONS(numIterations); + } + + if (!(error && error != ntsa::Error::e_WOULD_BLOCK) && + error != ntsa::Error::e_EOF) + { + this->privateRearmAfterReceive(self); + } } } - if (numIterations > 0) { - NTCS_METRICS_UPDATE_RECEIVE_ITERATIONS(numIterations); + if (encrypted) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; + } + + if (!d_shutdownState.canReceive()) { + return; + } + + bsl::size_t numIterations = 0; + + while (true) { + ++numIterations; + + error = this->privateSocketReadableIteration(self, true); + if (error) { + break; + } + + if (!d_receiveGreedily) { + break; + } + + if (!d_shutdownState.canReceive()) { + break; + } + } + + if (numIterations > 0) { + NTCS_METRICS_UPDATE_RECEIVE_ITERATIONS(numIterations); + } + + if (error == ntsa::Error::e_EOF) { + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); + this->privateFail(self, error); + } + else if (error && error != ntsa::Error::e_WOULD_BLOCK) { + this->privateFail(self, error); + } + else { + this->privateRearmAfterReceive(self); + } + + return; } - if (error && error != ntsa::Error::e_WOULD_BLOCK) { + // Perform any cross-cutting shutdown or failure sequence outside the + // receive mutex, acquiring all three mutexes in canonical order. + + if (error == ntsa::Error::e_EOF) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); this->privateFail(self, error); } - else { - this->privateRearmAfterReceive(self); + else if (error && error != ntsa::Error::e_WOULD_BLOCK) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + this->privateFail(self, error); } } @@ -345,21 +446,38 @@ void StreamSocket::processSocketWritable(const ntca::ReactorEvent& event) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); - NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + // These lifecycle predicates are atomic and may be read without any lock. + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { return; } if (NTCCFG_UNLIKELY(d_connectInProgress)) { - this->privateSocketWritableConnection(self); + // Connection establishment is a cross-cutting transition (it sets up + // both the send and receive paths and announces establishment); + // acquire all three mutexes in canonical order. + + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; + } + + if (d_connectInProgress) { + this->privateSocketWritableConnection(self); + } + return; } @@ -368,34 +486,72 @@ void StreamSocket::processSocketWritable(const ntca::ReactorEvent& event) } ntsa::Error error; - bsl::size_t numIterations = 0; + bool shutdownSendPending = false; - while (d_sendQueue.hasEntry()) { - ++numIterations; + { + LockGuard sendLock(&d_sendMutex); - error = this->privateSocketWritableIteration(self); - if (error) { - break; + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; } - if (!d_sendGreedily) { - break; + if (!d_shutdownState.canSend()) { + return; } - if (!d_shutdownState.canSend()) { - break; + bsl::size_t numIterations = 0; + + while (d_sendQueue.hasEntry()) { + ++numIterations; + + error = this->privateSocketWritableIteration(self, + &shutdownSendPending); + if (error) { + break; + } + + if (shutdownSendPending) { + break; + } + + if (!d_sendGreedily) { + break; + } + + if (!d_shutdownState.canSend()) { + break; + } } - } - if (numIterations > 0) { - NTCS_METRICS_UPDATE_SEND_ITERATIONS(numIterations); + if (numIterations > 0) { + NTCS_METRICS_UPDATE_SEND_ITERATIONS(numIterations); + } + + if (!(error && error != ntsa::Error::e_WOULD_BLOCK) && + !shutdownSendPending) + { + this->privateRearmAfterSend(self); + } } + // Perform any cross-cutting failure or shutdown sequence outside the send + // mutex, acquiring all three mutexes in canonical order. + if (error && error != ntsa::Error::e_WOULD_BLOCK) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + this->privateFail(self, error); } - else { - this->privateRearmAfterSend(self); + else if (shutdownSendPending) { + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); + + this->privateShutdownSend(self, true); } } @@ -405,7 +561,12 @@ void StreamSocket::processSocketError(const ntca::ReactorEvent& event) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Error handling is cross-cutting (it may fail a connect, an upgrade, or + // the established connection); acquire all three mutexes in canonical + // order. + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -436,7 +597,9 @@ void StreamSocket::processNotifications( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Notifications carry zero-copy completions and transmit timestamps, both + // of which belong to the send path. + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -475,7 +638,11 @@ void StreamSocket::processConnectDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Connect failure is cross-cutting; acquire all three mutexes in canonical + // order. + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -517,7 +684,11 @@ void StreamSocket::processConnectRetryTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Connect retry/failure is cross-cutting; acquire all three mutexes in + // canonical order. + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -555,7 +726,11 @@ void StreamSocket::processUpgradeTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Upgrade failure is cross-cutting; acquire all three mutexes in canonical + // order. + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -582,7 +757,7 @@ void StreamSocket::processSendRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -611,7 +786,7 @@ void StreamSocket::processSendRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_sendMutex); } } } @@ -627,7 +802,7 @@ void StreamSocket::processSendDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -661,7 +836,7 @@ void StreamSocket::processSendDeadlineTimer( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } } @@ -676,7 +851,7 @@ void StreamSocket::processReceiveRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -705,7 +880,7 @@ void StreamSocket::processReceiveRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -721,7 +896,7 @@ void StreamSocket::processReceiveDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -749,7 +924,7 @@ void StreamSocket::processReceiveDeadlineTimer( d_reactorStrand_sp, self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -759,8 +934,9 @@ void StreamSocket::privateEncryptionHandshake( const bsl::shared_ptr& certificate, const bsl::string& details) { - // IMPLEMENTATION NOTE: This function is always called under a lock - // during the exection of 'd_encryption->popIncomingCipherText()'. + // IMPLEMENTATION NOTE: This function is always called while holding + // 'd_mutex', 'd_sendMutex', and 'd_receiveMutex' (either from the encrypted + // read path or from 'privateUpgrade'), so all callbacks are deferred. NTCI_LOG_CONTEXT(); @@ -790,7 +966,7 @@ void StreamSocket::privateEncryptionHandshake( event, d_reactorStrand_sp, self, - false, + true, &d_mutex); } } @@ -802,7 +978,8 @@ void StreamSocket::privateEncryptionHandshake( } ntsa::Error StreamSocket::privateSocketReadableIteration( - const bsl::shared_ptr& self) + const bsl::shared_ptr& self, + bool defer) { NTCI_LOG_CONTEXT(); @@ -919,8 +1096,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( receiveEvent, d_reactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } if (d_receiveQueue.authorizeLowWatermarkEvent()) { @@ -939,8 +1116,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } @@ -952,7 +1129,7 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + defer, false); if (d_session_sp) { @@ -966,8 +1143,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } @@ -1138,6 +1315,9 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( NTCI_LOG_TRACE("Connection attempt succeeded"); + // Connection establishment runs while holding all three mutexes, so all + // announcements and callbacks are deferred. + if (d_session_sp) { ntcs::Dispatch::announceConnectComplete(d_session_sp, self, @@ -1145,7 +1325,7 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, + true, &d_mutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { @@ -1158,7 +1338,7 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( connectEvent, d_reactorStrand_sp, self, - false, + true, &d_mutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { @@ -1171,14 +1351,15 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( d_managerStrand_sp, d_reactorStrand_sp, self, - false, + true, &d_mutex); return ntsa::Error(); } ntsa::Error StreamSocket::privateSocketWritableIteration( - const bsl::shared_ptr& self) + const bsl::shared_ptr& self, + bool* shutdownSendPending) { if (!d_sendQueue.hasEntry()) { return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -1190,7 +1371,8 @@ ntsa::Error StreamSocket::privateSocketWritableIteration( return this->privateSocketWritableIterationBatch(self); } else { - return this->privateSocketWritableIterationFront(self); + return this->privateSocketWritableIterationFront(self, + shutdownSendPending); } } @@ -1313,7 +1495,7 @@ ntsa::Error StreamSocket::privateSocketWritableIterationBatch( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } @@ -1334,7 +1516,7 @@ ntsa::Error StreamSocket::privateSocketWritableIterationBatch( d_reactorStrand_sp, self, true, - &d_mutex); + &d_sendMutex); } } @@ -1350,7 +1532,8 @@ ntsa::Error StreamSocket::privateSocketWritableIterationBatch( } ntsa::Error StreamSocket::privateSocketWritableIterationFront( - const bsl::shared_ptr& self) + const bsl::shared_ptr& self, + bool* shutdownSendPending) { NTCI_LOG_CONTEXT(); @@ -1439,7 +1622,7 @@ ntsa::Error StreamSocket::privateSocketWritableIterationFront( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } if (d_sendQueue.authorizeLowWatermarkEvent()) { @@ -1460,13 +1643,17 @@ ntsa::Error StreamSocket::privateSocketWritableIterationFront( d_reactorStrand_sp, self, true, - &d_mutex); + &d_sendMutex); } } } else { + // A graceful shutdown sentinel has been drained. Defer the actual + // shutdown-for-sending to the caller ('processSocketWritable'), which + // will perform it after releasing the send mutex so the cross-cutting + // shutdown sequence can acquire the mutexes in canonical order. d_sendQueue.popEntry(); - this->privateShutdownSend(self, false); + *shutdownSendPending = true; } if (!d_sendQueue.hasEntry()) { @@ -1631,7 +1818,12 @@ void StreamSocket::privateFailConnectPart2( NTCI_LOG_CONTEXT(); if (lock) { + // Invoked asynchronously as the reactor detachment callback. Acquire + // all three mutexes in canonical order because connect failure cleanup + // touches the send and receive paths and the shared state. d_mutex.lock(); + d_sendMutex.lock(); + d_receiveMutex.lock(); BSLS_ASSERT(d_detachState.mode() == ntcs::DetachMode::e_INITIATED); d_detachState.setMode(ntcs::DetachMode::e_IDLE); } @@ -1664,6 +1856,11 @@ void StreamSocket::privateFailConnectPart2( } } + NTCCFG_WARNING_UNUSED(defer); + + // This cleanup runs while holding all three mutexes, so all announcements + // and callbacks are forced to be deferred. + if (d_session_sp) { ntcs::Dispatch::announceConnectComplete(d_session_sp, self, @@ -1671,7 +1868,7 @@ void StreamSocket::privateFailConnectPart2( d_sessionStrand_sp, ntci::Strand::unknown(), self, - defer, + true, &d_mutex); } @@ -1680,7 +1877,7 @@ void StreamSocket::privateFailConnectPart2( connectEvent, d_reactorStrand_sp, self, - defer, + true, &d_mutex); } @@ -1716,6 +1913,8 @@ void StreamSocket::privateFailConnectPart2( d_deferredCalls.clear(); if (lock) { + d_receiveMutex.unlock(); + d_sendMutex.unlock(); d_mutex.unlock(); } } @@ -1754,7 +1953,7 @@ void StreamSocket::privateFailUpgrade( this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1764,14 +1963,14 @@ void StreamSocket::privateFailUpgrade( upgradeEvent, d_reactorStrand_sp, self, - false, + true, &d_mutex); } this->privateShutdown(self, ntsa::ShutdownType::e_BOTH, ntsa::ShutdownMode::e_IMMEDIATE, - false); + true); } void StreamSocket::privateFail(const bsl::shared_ptr& self, @@ -1790,7 +1989,7 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1832,14 +2031,14 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, d_sessionStrand_sp, d_reactorStrand_sp, self, - false, + true, &d_mutex); } this->privateShutdown(self, ntsa::ShutdownType::e_BOTH, ntsa::ShutdownMode::e_IMMEDIATE, - false); + true); } } @@ -2027,7 +2226,12 @@ void StreamSocket::privateShutdownSequenceComplete( NTCI_LOG_CONTEXT(); if (lock) { + // Invoked asynchronously as the reactor detachment callback. Acquire + // all three mutexes in canonical order because the shutdown completion + // touches both the send and receive paths as well as the shared state. d_mutex.lock(); + d_sendMutex.lock(); + d_receiveMutex.lock(); BSLS_ASSERT(d_detachState.mode() == ntcs::DetachMode::e_INITIATED); d_detachState.setMode(ntcs::DetachMode::e_IDLE); } @@ -2371,6 +2575,8 @@ void StreamSocket::privateShutdownSequenceComplete( d_deferredCalls.clear(); if (lock) { + d_receiveMutex.unlock(); + d_sendMutex.unlock(); d_mutex.unlock(); } } @@ -2397,9 +2603,16 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( break; } - ntcs::FlowControlContext context; - if (d_flowControlState.relax(&context, direction, unlock)) { - if (relaxSend) { + // The send-direction flow control state is guarded by 'd_sendMutex' and the + // receive-direction flow control state by 'd_receiveMutex'; the caller must + // hold the corresponding leaf mutex for each direction being relaxed. + + if (relaxSend) { + ntcs::FlowControlContext context; + if (d_flowControlState.relax(&context, + ntca::FlowControlType::e_SEND, + unlock)) + { if (context.enableSend()) { if (d_shutdownState.canSend()) { ntcs::ObserverRef reactorRef(&d_reactor); @@ -2422,13 +2635,19 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } } + } - if (relaxReceive) { + if (relaxReceive) { + ntcs::FlowControlContext context; + if (d_flowControlState.relax(&context, + ntca::FlowControlType::e_RECEIVE, + unlock)) + { if (context.enableReceive()) { if (d_shutdownState.canReceive()) { ntcs::ObserverRef reactorRef(&d_reactor); @@ -2451,7 +2670,7 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2486,9 +2705,16 @@ ntsa::Error StreamSocket::privateApplyFlowControl( break; } - ntcs::FlowControlContext context; - if (d_flowControlState.apply(&context, direction, lock)) { - if (applySend) { + // The send-direction flow control state is guarded by 'd_sendMutex' and the + // receive-direction flow control state by 'd_receiveMutex'; the caller must + // hold the corresponding leaf mutex for each direction being applied. + + if (applySend) { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_SEND, + lock)) + { if (!context.enableSend()) { ntcs::ObserverRef reactorRef(&d_reactor); if (reactorRef) { @@ -2509,12 +2735,18 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } + } - if (applyReceive) { + if (applyReceive) { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_RECEIVE, + lock)) + { if (!context.enableReceive()) { ntcs::ObserverRef reactorRef(&d_reactor); if (reactorRef) { @@ -2535,7 +2767,7 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2549,15 +2781,16 @@ bool StreamSocket::privateCloseFlowControl( bool defer, const ntci::SocketDetachedCallback& detachCallback) { - bool applySend = true; - bool applyReceive = true; + // This function is only invoked from the cross-cutting shutdown sequence, + // which holds 'd_mutex', 'd_sendMutex', and 'd_receiveMutex'. Both + // direction-scoped flow control states are therefore closed here. - ntcs::FlowControlContext context; - if (d_flowControlState.apply(&context, - ntca::FlowControlType::e_BOTH, - true)) { - if (applySend) { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_SEND, + true)) + { if (!context.enableSend()) { if (d_session_sp) { ntca::WriteQueueEvent event; @@ -2573,12 +2806,18 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } + } - if (applyReceive) { + { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_RECEIVE, + true)) + { if (!context.enableReceive()) { if (d_session_sp) { ntca::ReadQueueEvent event; @@ -2594,7 +2833,7 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2635,7 +2874,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_SEND, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canSend()) { @@ -2703,7 +2942,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canReceive()) { @@ -3219,7 +3458,7 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, + true, &d_mutex); } } @@ -3239,7 +3478,7 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, + true, &d_mutex); } } @@ -3265,7 +3504,7 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, + true, &d_mutex); } } @@ -3317,7 +3556,7 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, + true, &d_mutex); } } @@ -3389,10 +3628,11 @@ ntsa::Error StreamSocket::privateDequeueReceiveBufferRaw( return error; } else if (error == ntsa::Error::e_EOF) { + // The shutdown of reception is performed by the caller + // ('processSocketReadable') after releasing the receive mutex, so + // that the cross-cutting shutdown sequence can acquire the mutexes + // in canonical order. NTCR_STREAMSOCKET_LOG_END_OF_FILE(); - this->privateShutdownReceive(self, - ntsa::ShutdownOrigin::e_REMOTE, - false); return error; } else { @@ -3417,10 +3657,11 @@ ntsa::Error StreamSocket::privateDequeueReceiveBufferRaw( d_totalBytesReceived += context->bytesReceived(); } else { + // The shutdown of reception is performed by the caller + // ('processSocketReadable') after releasing the receive mutex, so that + // the cross-cutting shutdown sequence can acquire the mutexes in + // canonical order. NTCR_STREAMSOCKET_LOG_END_OF_FILE(); - this->privateShutdownReceive(self, - ntsa::ShutdownOrigin::e_REMOTE, - false); return ntsa::Error(ntsa::Error::e_EOF); } @@ -3524,7 +3765,7 @@ ntsa::Error StreamSocket::privateSendRaw( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -3660,7 +3901,7 @@ ntsa::Error StreamSocket::privateSendRaw( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -3771,10 +4012,16 @@ ntsa::Error StreamSocket::privateSendEncrypted( } if (cipherData.length() > 0) { + // The encrypted send path holds both 'd_mutex' and 'd_sendMutex'; + // suppress synchronous (recursive) completion so the raw send does not + // dispatch a callback while holding more than one mutex. + ntca::SendOptions rawOptions(options); + rawOptions.setRecurse(false); + error = this->privateSendRaw(self, cipherData, state, - options, + rawOptions, setting, callback); if (error) { @@ -3786,14 +4033,14 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - const bool defer = !options.recurse(); - + // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so + // the completion callback is always deferred. callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - defer, - &d_mutex); + true, + &d_sendMutex); } return ntsa::Error(); @@ -3824,10 +4071,16 @@ ntsa::Error StreamSocket::privateSendEncrypted( } if (cipherData.length() > 0) { + // The encrypted send path holds both 'd_mutex' and 'd_sendMutex'; + // suppress synchronous (recursive) completion so the raw send does not + // dispatch a callback while holding more than one mutex. + ntca::SendOptions rawOptions(options); + rawOptions.setRecurse(false); + error = this->privateSendRaw(self, cipherData, state, - options, + rawOptions, setting, callback); if (error) { @@ -3839,14 +4092,14 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - const bool defer = !options.recurse(); - + // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so + // the completion callback is always deferred. callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - defer, - &d_mutex); + true, + &d_sendMutex); } return ntsa::Error(); @@ -4256,7 +4509,9 @@ void StreamSocket::processRemoteEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -4357,7 +4612,7 @@ void StreamSocket::processRemoteEndpointResolution( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, + true, &d_mutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == @@ -4894,7 +5149,7 @@ void StreamSocket::privateRetryConnectToEndpoint( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, + true, &d_mutex); } @@ -5230,7 +5485,7 @@ void StreamSocket::privateZeroCopyUpdate( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } } @@ -5283,6 +5538,8 @@ StreamSocket::StreamSocket( bslma::Allocator* basicAllocator) : d_object("ntcr::StreamSocket") , d_mutex() +, d_sendMutex() +, d_receiveMutex() , d_transport(ntsa::Transport::e_UNDEFINED) , d_systemHandle(ntsa::k_INVALID_HANDLE) , d_systemSourceEndpoint() @@ -5936,7 +6193,9 @@ ntsa::Error StreamSocket::upgrade( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6094,8 +6353,29 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Fast path: an unencrypted send only needs the send mutex. The encryption + // object is co-guarded and may be examined under the send mutex; if it is + // present, escalate to also holding the state mutex (in canonical order), + // since the encrypted send path operates on the shared encryption object. + + { + LockGuard sendLock(&d_sendMutex); + if (NTCCFG_LIKELY(!d_encryption_sp)) { + return this->privateSend(self, data, options, callback); + } + } + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + return this->privateSend(self, data, options, callback); +} + +ntsa::Error StreamSocket::privateSend( + const bsl::shared_ptr& self, + const bdlbb::Blob& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback) +{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -6137,7 +6417,7 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -6221,8 +6501,27 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Fast path: an unencrypted send only needs the send mutex; escalate to + // also hold the state mutex (in canonical order) only when encrypted. + { + LockGuard sendLock(&d_sendMutex); + if (NTCCFG_LIKELY(!d_encryption_sp)) { + return this->privateSend(self, data, options, callback); + } + } + + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + return this->privateSend(self, data, options, callback); +} + +ntsa::Error StreamSocket::privateSend( + const bsl::shared_ptr& self, + const ntsa::Data& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback) +{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -6264,7 +6563,7 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -6339,7 +6638,7 @@ ntsa::Error StreamSocket::receive(ntca::ReceiveContext* context, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6474,7 +6773,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6588,7 +6887,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); bool receiveQueueHighWatermarkViolatedAfter = d_receiveQueue.isHighWatermarkViolated(); @@ -6667,7 +6966,9 @@ ntsa::Error StreamSocket::registerManager( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (manager) { d_manager_sp = manager; @@ -6687,7 +6988,9 @@ ntsa::Error StreamSocket::registerManager( ntsa::Error StreamSocket::deregisterManager() { - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); d_manager_sp.reset(); d_managerStrand_sp.reset(); @@ -6700,7 +7003,9 @@ ntsa::Error StreamSocket::registerSession( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (session) { d_session_sp = session; @@ -6730,7 +7035,9 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (callback) { bsl::shared_ptr session; @@ -6767,7 +7074,9 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (callback) { bsl::shared_ptr session; @@ -6797,7 +7106,9 @@ ntsa::Error StreamSocket::registerSessionCallback( ntsa::Error StreamSocket::deregisterSession() { - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); d_session_sp.reset(); d_sessionStrand_sp.reset(); @@ -6808,7 +7119,7 @@ ntsa::Error StreamSocket::deregisterSession() ntsa::Error StreamSocket::setZeroCopyThreshold(bsl::size_t value) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return this->privateZeroCopyEngage(self, value); } @@ -6824,7 +7135,7 @@ ntsa::Error StreamSocket::setConnectRateLimiter( ntsa::Error StreamSocket::setWriteDeflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); d_sendDeflater_sp = compression; @@ -6836,7 +7147,7 @@ ntsa::Error StreamSocket::setWriteRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6865,7 +7176,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6904,7 +7215,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6944,7 +7255,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7005,7 +7316,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntsa::Error StreamSocket::setReadInflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_receiveInflater_sp = compression; @@ -7017,7 +7328,7 @@ ntsa::Error StreamSocket::setReadRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7046,7 +7357,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7087,7 +7398,7 @@ ntsa::Error StreamSocket::setReadQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7113,7 +7424,7 @@ ntsa::Error StreamSocket::setReadQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7146,7 +7457,7 @@ ntsa::Error StreamSocket::timestampOutgoingData(bool enable) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return privateTimestampOutgoingData(self, enable); } @@ -7155,7 +7466,7 @@ ntsa::Error StreamSocket::timestampIncomingData(bool enable) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return privateTimestampIncomingData(self, enable); } @@ -7165,7 +7476,9 @@ ntsa::Error StreamSocket::relaxFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7182,7 +7495,9 @@ ntsa::Error StreamSocket::applyFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7224,7 +7539,9 @@ ntsa::Error StreamSocket::cancel(const ntca::ConnectToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7250,7 +7567,9 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7299,7 +7618,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7345,7 +7664,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7386,7 +7705,9 @@ ntsa::Error StreamSocket::downgrade() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); return this->privateDowngrade(self, ntca::DowngradeOptions()); } @@ -7395,7 +7716,9 @@ ntsa::Error StreamSocket::downgrade(const ntca::DowngradeOptions& options) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); if (options.abortive().isNull() || !options.abortive().value()) { return this->privateDowngrade(self, options); @@ -7410,7 +7733,9 @@ ntsa::Error StreamSocket::shutdown(ntsa::ShutdownType::Value direction, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7459,7 +7784,9 @@ ntsa::Error StreamSocket::release(ntsa::Handle* result, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); *result = ntsa::k_INVALID_HANDLE; @@ -7495,7 +7822,9 @@ void StreamSocket::close(const ntci::CloseCallback& callback) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard stateLock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + LockGuard receiveLock(&d_receiveMutex); this->privateClose(self, callback); } @@ -7700,49 +8029,49 @@ bsl::size_t StreamSocket::threadIndex() const bsl::size_t StreamSocket::readQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.size(); } bsl::size_t StreamSocket::readQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.lowWatermark(); } bsl::size_t StreamSocket::readQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.highWatermark(); } bsl::size_t StreamSocket::writeQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.size(); } bsl::size_t StreamSocket::writeQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.lowWatermark(); } bsl::size_t StreamSocket::writeQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.highWatermark(); } bsl::size_t StreamSocket::totalBytesSent() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_totalBytesSent; } bsl::size_t StreamSocket::totalBytesReceived() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_totalBytesReceived; } @@ -7786,11 +8115,16 @@ void StreamSocket::getInfo(ntsa::SocketInfo* result) const d_publicRemoteEndpoint.load(&remoteEndpoint); { - LockGuard lock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + sendQueueSize = d_sendQueue.size(); + } - sendQueueSize = d_sendQueue.size(); + { + LockGuard receiveLock(&d_receiveMutex); receiveQueueSize = d_receiveQueue.size(); + } + { if (d_shutdownState.completed()) { socketState = ntsa::SocketState::e_CLOSED; } diff --git a/groups/ntc/ntcr/ntcr_streamsocket.h b/groups/ntc/ntcr/ntcr_streamsocket.h index 2f6eae2e..74b40d45 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.h +++ b/groups/ntc/ntcr/ntcr_streamsocket.h @@ -91,7 +91,38 @@ class StreamSocket : public ntci::StreamSocket, typedef ntccfg::LockGuard LockGuard; ntccfg::Object d_object; + // LOCKING STRATEGY + // + // 'd_mutex' (the "state" mutex) guards the shared lifecycle and control + // state of the socket: the connect/upgrade machinery, the open, shutdown, + // and detach state machines' *transitions*, and the encryption object. + // + // 'd_sendMutex' guards the write path (the send queue, zero-copy queue, + // outgoing timestamping, and send-direction reactor interest). + // 'd_receiveMutex' guards the read path (the receive queue and + // receive-direction reactor interest). These two leaf mutexes are + // independent so that sending is not serialized against receiving. + // + // 'd_flowControlState' is itself thread safe (lock-free), so a single + // instance is shared by both directions: the send path applies/relaxes it + // with 'e_SEND' under 'd_sendMutex' and the receive path with 'e_RECEIVE' + // under 'd_receiveMutex'. + // + // When more than one is held they are always acquired in the order + // 'd_mutex' -> 'd_sendMutex' -> 'd_receiveMutex'. A steady-state I/O drain + // holds only its own leaf; when it must perform a cross-cutting lifecycle + // transition (connection establishment, failure, shutdown) it first + // releases its leaf, then re-acquires in canonical order. + // + // The observable lifecycle predicates ('d_openState', 'd_shutdownState', + // 'd_detachState.mode()') are atomic and may be read without any lock. The + // co-guarded members ('d_socket_sp', 'd_encryption_sp', 'd_session_sp', + // 'd_sessionStrand_sp', 'd_manager_sp', 'd_managerStrand_sp') are written + // only while holding all three mutexes and may therefore be read while + // holding any one of them. mutable Mutex d_mutex; + mutable Mutex d_sendMutex; + mutable Mutex d_receiveMutex; ntsa::Transport::Value d_transport; ntsa::Handle d_systemHandle; ntsa::Endpoint d_systemSourceEndpoint; @@ -147,7 +178,7 @@ class StreamSocket : public ntci::StreamSocket, bsl::shared_ptr d_connectRetryTimer_sp; bsl::shared_ptr d_connectRateLimiter_sp; bsl::shared_ptr d_connectRateTimer_sp; - bool d_connectInProgress; + bsls::AtomicBool d_connectInProgress; ntca::UpgradeOptions d_upgradeOptions; ntci::UpgradeCallback d_upgradeCallback; bsl::shared_ptr d_upgradeTimer_sp; @@ -241,9 +272,13 @@ class StreamSocket : public ntci::StreamSocket, const bsl::string& details); /// Process the readability of the socket by performing one read - /// iteration. + /// iteration. If the specified 'defer' flag is true, force all event + /// announcements and callback invocations made during the iteration to be + /// deferred (used when the iteration runs while holding more than one + /// mutex, e.g. the encrypted read path). ntsa::Error privateSocketReadableIteration( - const bsl::shared_ptr& self); + const bsl::shared_ptr& self, + bool defer); /// Process the writability of the socket indicating the connection /// is established. @@ -251,9 +286,14 @@ class StreamSocket : public ntci::StreamSocket, const bsl::shared_ptr& self); /// Process the writability of the socket by performing one write - /// iteration. + /// iteration. If a graceful shutdown sentinel is drained during the + /// iteration, set the specified '*shutdownSendPending' to true instead of + /// shutting down for sending inline, so that the caller + /// ('processSocketWritable') can perform the cross-cutting shutdown + /// sequence after releasing the send mutex. ntsa::Error privateSocketWritableIteration( - const bsl::shared_ptr& self); + const bsl::shared_ptr& self, + bool* shutdownSendPending); /// Process the writability of the socket by performing one write /// iteration from the contiguous range of suitable entries at the front @@ -262,9 +302,12 @@ class StreamSocket : public ntci::StreamSocket, const bsl::shared_ptr& self); /// Process the writability of the socket by performing one write - /// iteration from the entry at the front of the write queue. + /// iteration from the entry at the front of the write queue. If a graceful + /// shutdown sentinel is drained, set the specified '*shutdownSendPending' + /// to true rather than shutting down for sending inline. ntsa::Error privateSocketWritableIterationFront( - const bsl::shared_ptr& self); + const bsl::shared_ptr& self, + bool* shutdownSendPending); /// Indicate a connection failure has occurred. If the specified 'defer' /// flag is true, ensure the announcement is deferred. If the specified @@ -417,6 +460,24 @@ class StreamSocket : public ntci::StreamSocket, ntsa::ReceiveContext* context, bdlbb::Blob* data); + /// Enqueue the specified 'data' for transmission according to the + /// specified 'options', invoking the specified 'callback' on completion. + /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the + /// socket is encrypted, 'd_mutex' as well). Return the error. + ntsa::Error privateSend(const bsl::shared_ptr& self, + const bdlbb::Blob& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback); + + /// Enqueue the specified 'data' for transmission according to the + /// specified 'options', invoking the specified 'callback' on completion. + /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the + /// socket is encrypted, 'd_mutex' as well). Return the error. + ntsa::Error privateSend(const bsl::shared_ptr& self, + const ntsa::Data& data, + const ntca::SendOptions& options, + const ntci::SendCallback& callback); + /// Rearm the interest in the writability of the socket in the reactor, /// if necessary. void privateRearmAfterSend(const bsl::shared_ptr& self); diff --git a/groups/ntc/ntcr/ntcr_streamsocket.t.cpp b/groups/ntc/ntcr/ntcr_streamsocket.t.cpp index 56139586..e388556e 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.t.cpp +++ b/groups/ntc/ntcr/ntcr_streamsocket.t.cpp @@ -4039,6 +4039,14 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica4) .ONCE() .RETURN(ntsa::Error::invalid()); + // The connect failure callback is now delivered deferred: the failure + // path holds more than one mutex, so the announcement is enqueued via + // the reactor rather than invoked synchronously. + ntci::Reactor::Functor deferredCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredCallback)); + ntca::ReactorEvent event; bsl::shared_ptr reactorSocket = socket; reactorSocket->processSocketWritable(event); @@ -4046,6 +4054,9 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica4) NTCI_LOG_DEBUG("Ensure that connection callback was called and " "connection error was indicated"); { + NTSCFG_TEST_TRUE(deferredCallback); + deferredCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -4238,12 +4249,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica5) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -4440,12 +4462,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica6) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5019,6 +5052,14 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica4) .ONCE() .RETURN(ntsa::Error::invalid()); + // The connect failure callback is now delivered deferred: the failure + // path holds more than one mutex, so the announcement is enqueued via + // the reactor rather than invoked synchronously. + ntci::Reactor::Functor deferredCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredCallback)); + ntca::ReactorEvent event; bsl::shared_ptr reactorSocket = socket; reactorSocket->processSocketWritable(event); @@ -5026,6 +5067,9 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica4) NTCI_LOG_DEBUG("Ensure that connection callback was called and " "connection error was indicated"); { + NTSCFG_TEST_TRUE(deferredCallback); + deferredCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5222,12 +5266,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica5) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5431,12 +5486,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica6) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5620,12 +5686,23 @@ NTSCFG_TEST_FUNCTION( .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); diff --git a/groups/ntc/ntcs/ntcs_detachstate.h b/groups/ntc/ntcs/ntcs_detachstate.h index 3044641a..ba5e24b4 100644 --- a/groups/ntc/ntcs/ntcs_detachstate.h +++ b/groups/ntc/ntcs/ntcs_detachstate.h @@ -21,6 +21,7 @@ BSLS_IDENT("$Id: $") #include #include +#include namespace BloombergLP { namespace ntcs { @@ -73,12 +74,14 @@ class DetachGoal /// Provide a enumeration that indicates state of a socket detachment process. /// /// @par Thread Safety -/// This class is not thread safe. +/// The mode is stored atomically: 'mode' may be called concurrently with a +/// single thread invoking 'setMode'. The goal is not synchronized and must +/// only be accessed while externally serialized. /// /// @ingroup module_ntcs class DetachState { - ntcs::DetachMode::Value d_mode; + bsls::AtomicInt d_mode; ntcs::DetachGoal::Value d_goal; private: @@ -121,7 +124,7 @@ DetachState::~DetachState() NTCCFG_INLINE void DetachState::setMode(ntcs::DetachMode::Value mode) { - d_mode = mode; + d_mode.storeRelease(mode); } NTCCFG_INLINE @@ -133,7 +136,7 @@ void DetachState::setGoal(ntcs::DetachGoal::Value goal) NTCCFG_INLINE ntcs::DetachMode::Value DetachState::mode() const { - return d_mode; + return static_cast(d_mode.loadAcquire()); } NTCCFG_INLINE diff --git a/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp b/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp index 782d37dc..96fc05ef 100644 --- a/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp +++ b/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp @@ -24,11 +24,7 @@ namespace BloombergLP { namespace ntcs { FlowControlState::FlowControlState() -: d_enableSend(false) -, d_enableReceive(false) -, d_lockSend(false) -, d_lockReceive(false) -, d_closed(false) +: d_flags(0) { } @@ -42,12 +38,6 @@ bool FlowControlState::apply(ntcs::FlowControlContext* context, { context->reset(); - if (d_closed) { - return false; - } - - bool result = false; - bool applySend = false; bool applyReceive = false; @@ -64,36 +54,58 @@ bool FlowControlState::apply(ntcs::FlowControlContext* context, break; } - if (applySend) { - if (!d_lockSend) { - if (d_enableSend) { - d_enableSend = false; - result = true; - } - } + // Commit the compound change atomically: read the current flags, compute + // the new flags and result from that snapshot, then compare-and-swap. If + // the swap loses a race with a concurrent operation, retry. + + for (;;) { + const unsigned int oldFlags = d_flags.load(); - if (lock) { - d_lockSend = true; + if (oldFlags & e_CLOSED) { + return false; } - } - if (applyReceive) { - if (!d_lockReceive) { - if (d_enableReceive) { - d_enableReceive = false; - result = true; + unsigned int newFlags = oldFlags; + bool result = false; + + if (applySend) { + if (!(newFlags & e_LOCK_SEND)) { + if (newFlags & e_ENABLE_SEND) { + newFlags &= ~static_cast(e_ENABLE_SEND); + result = true; + } + } + + if (lock) { + newFlags |= e_LOCK_SEND; } } - if (lock) { - d_lockReceive = true; + if (applyReceive) { + if (!(newFlags & e_LOCK_RECEIVE)) { + if (newFlags & e_ENABLE_RECEIVE) { + newFlags &= ~static_cast(e_ENABLE_RECEIVE); + result = true; + } + } + + if (lock) { + newFlags |= e_LOCK_RECEIVE; + } } - } - context->setEnableSend(d_enableSend); - context->setEnableReceive(d_enableReceive); + if (newFlags == oldFlags) { + context->setEnableSend((oldFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((oldFlags & e_ENABLE_RECEIVE) != 0); + return result; + } - return result; + if (d_flags.testAndSwap(oldFlags, newFlags) == oldFlags) { + context->setEnableSend((newFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((newFlags & e_ENABLE_RECEIVE) != 0); + return result; + } + } } bool FlowControlState::relax(ntcs::FlowControlContext* context, @@ -102,12 +114,6 @@ bool FlowControlState::relax(ntcs::FlowControlContext* context, { context->reset(); - if (d_closed) { - return false; - } - - bool result = false; - bool relaxSend = false; bool relaxReceive = false; @@ -124,54 +130,67 @@ bool FlowControlState::relax(ntcs::FlowControlContext* context, break; } - if (relaxSend) { - if (unlock) { - d_lockSend = false; + for (;;) { + const unsigned int oldFlags = d_flags.load(); + + if (oldFlags & e_CLOSED) { + return false; } - if (!d_lockSend) { - if (!d_enableSend) { - d_enableSend = true; - result = true; + unsigned int newFlags = oldFlags; + bool result = false; + + if (relaxSend) { + if (unlock) { + newFlags &= ~static_cast(e_LOCK_SEND); } - } - } - if (relaxReceive) { - if (unlock) { - d_lockReceive = false; + if (!(newFlags & e_LOCK_SEND)) { + if (!(newFlags & e_ENABLE_SEND)) { + newFlags |= e_ENABLE_SEND; + result = true; + } + } } - if (!d_lockReceive) { - if (!d_enableReceive) { - d_enableReceive = true; - result = true; + if (relaxReceive) { + if (unlock) { + newFlags &= ~static_cast(e_LOCK_RECEIVE); + } + + if (!(newFlags & e_LOCK_RECEIVE)) { + if (!(newFlags & e_ENABLE_RECEIVE)) { + newFlags |= e_ENABLE_RECEIVE; + result = true; + } } } - } - context->setEnableSend(d_enableSend); - context->setEnableReceive(d_enableReceive); + if (newFlags == oldFlags) { + context->setEnableSend((oldFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((oldFlags & e_ENABLE_RECEIVE) != 0); + return result; + } - return result; + if (d_flags.testAndSwap(oldFlags, newFlags) == oldFlags) { + context->setEnableSend((newFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((newFlags & e_ENABLE_RECEIVE) != 0); + return result; + } + } } void FlowControlState::close() { - d_enableSend = false; - d_lockSend = false; - d_enableReceive = false; - d_lockReceive = false; - d_closed = true; + // Clear the enable and lock bits and set the closed bit. This is a full + // overwrite to a constant value, so a single atomic store is sufficient; + // any concurrent compare-and-swap will observe the change and retry. + d_flags.store(e_CLOSED); } void FlowControlState::reset() { - d_enableSend = false; - d_enableReceive = false; - d_lockSend = false; - d_lockReceive = false; - d_closed = false; + d_flags.store(0); } bool FlowControlState::rearm(ntcs::FlowControlContext* context, @@ -184,7 +203,9 @@ bool FlowControlState::rearm(ntcs::FlowControlContext* context, return false; } - if (d_closed) { + const unsigned int flags = d_flags.load(); + + if (flags & e_CLOSED) { return false; } @@ -207,21 +228,21 @@ bool FlowControlState::rearm(ntcs::FlowControlContext* context, } if (rearmSend) { - if (d_enableSend) { - BSLS_ASSERT(!d_lockSend); + if (flags & e_ENABLE_SEND) { + BSLS_ASSERT(!(flags & e_LOCK_SEND)); result = true; } } if (rearmReceive) { - if (d_enableReceive) { - BSLS_ASSERT(!d_lockReceive); + if (flags & e_ENABLE_RECEIVE) { + BSLS_ASSERT(!(flags & e_LOCK_RECEIVE)); result = true; } } - context->setEnableSend(d_enableSend); - context->setEnableReceive(d_enableReceive); + context->setEnableSend((flags & e_ENABLE_SEND) != 0); + context->setEnableReceive((flags & e_ENABLE_RECEIVE) != 0); return result; } diff --git a/groups/ntc/ntcs/ntcs_flowcontrolstate.h b/groups/ntc/ntcs/ntcs_flowcontrolstate.h index 1ba39fa7..e4d2ecb6 100644 --- a/groups/ntc/ntcs/ntcs_flowcontrolstate.h +++ b/groups/ntc/ntcs/ntcs_flowcontrolstate.h @@ -24,7 +24,6 @@ BSLS_IDENT("$Id: $") #include #include #include -#include namespace BloombergLP { namespace ntcs { @@ -33,16 +32,26 @@ namespace ntcs { /// Provide a mechanism to to manage flow control. /// /// @par Thread Safety -/// This class is not thread safe. +/// This class is thread safe. The entire flow control state is stored in a +/// single atomic word, and each manipulator commits its compound change +/// atomically using a compare-and-swap loop, so the class is safe for +/// concurrent use without any external synchronization. /// /// @ingroup module_ntcs class FlowControlState { - bool d_enableSend; - bool d_enableReceive; - bool d_lockSend; - bool d_lockReceive; - bool d_closed; + /// This enumeration defines the bit within 'd_flags' that stores each + /// component of the flow control state. + enum Flag { + e_ENABLE_SEND = 1 << 0, + e_ENABLE_RECEIVE = 1 << 1, + e_LOCK_SEND = 1 << 2, + e_LOCK_RECEIVE = 1 << 3, + e_CLOSED = 1 << 4 + }; + + // The flow control state, storing the flags enumerated by 'Flag'. + bsls::AtomicUint d_flags; private: FlowControlState(const FlowControlState&) BSLS_KEYWORD_DELETED; @@ -119,31 +128,31 @@ class FlowControlState NTCCFG_INLINE bool FlowControlState::wantSend() const { - return d_enableSend; + return (d_flags.load() & e_ENABLE_SEND) != 0; } NTCCFG_INLINE bool FlowControlState::wantReceive() const { - return d_enableReceive; + return (d_flags.load() & e_ENABLE_RECEIVE) != 0; } NTCCFG_INLINE bool FlowControlState::lockSend() const { - return d_lockSend; + return (d_flags.load() & e_LOCK_SEND) != 0; } NTCCFG_INLINE bool FlowControlState::lockReceive() const { - return d_lockReceive; + return (d_flags.load() & e_LOCK_RECEIVE) != 0; } NTCCFG_INLINE bool FlowControlState::closed() const { - return d_closed; + return (d_flags.load() & e_CLOSED) != 0; } } // end namespace ntcs diff --git a/groups/ntc/ntcs/ntcs_openstate.cpp b/groups/ntc/ntcs/ntcs_openstate.cpp index 7ee2ec4f..2fb26fcc 100644 --- a/groups/ntc/ntcs/ntcs_openstate.cpp +++ b/groups/ntc/ntcs/ntcs_openstate.cpp @@ -25,7 +25,7 @@ namespace ntcs { const char* OpenState::toString() const { - switch (d_value) { + switch (static_cast(d_value.loadAcquire())) { case e_DEFAULT: return "DEFAULT"; case e_WAITING: diff --git a/groups/ntc/ntcs/ntcs_openstate.h b/groups/ntc/ntcs/ntcs_openstate.h index fb19c233..94c12968 100644 --- a/groups/ntc/ntcs/ntcs_openstate.h +++ b/groups/ntc/ntcs/ntcs_openstate.h @@ -21,6 +21,7 @@ BSLS_IDENT("$Id: $") #include #include +#include namespace BloombergLP { namespace ntcs { @@ -32,7 +33,9 @@ namespace ntcs { /// stream socket when connecting to a remote peer. /// /// @par Thread Safety -/// This class is not thread safe. +/// The state value is stored atomically: the accessors may be called +/// concurrently with each other and with a single thread invoking 'set', +/// but concurrent invocations of 'set' are not safe. /// /// @ingroup module_ntcs class OpenState @@ -57,7 +60,7 @@ class OpenState }; private: - Value d_value; + bsls::AtomicInt d_value; private: OpenState(const OpenState&) BSLS_KEYWORD_DELETED; @@ -126,61 +129,64 @@ OpenState::~OpenState() NTCCFG_INLINE void OpenState::set(Value value) { - d_value = value; + d_value.storeRelease(value); } NTCCFG_INLINE OpenState::Value OpenState::value() const { - return d_value; + return static_cast(d_value.loadAcquire()); } NTCCFG_INLINE bool OpenState::canBind() const { - return d_value == e_DEFAULT; + return d_value.loadAcquire() == e_DEFAULT; } NTCCFG_INLINE bool OpenState::canConnect() const { - return d_value == e_DEFAULT; + return d_value.loadAcquire() == e_DEFAULT; } NTCCFG_INLINE bool OpenState::canSend() const { - return d_value == e_CONNECTED; + return d_value.loadAcquire() == e_CONNECTED; } NTCCFG_INLINE bool OpenState::canReceive() const { - return d_value == e_CONNECTED || d_value == e_CLOSED; + const int value = d_value.loadAcquire(); + return value == e_CONNECTED || value == e_CLOSED; } NTCCFG_INLINE bool OpenState::is(Value value) const { - return d_value == value; + return d_value.loadAcquire() == value; } NTCCFG_INLINE bool OpenState::isEither(Value value1, Value value2) const { - return d_value == value1 || d_value == value2; + const int value = d_value.loadAcquire(); + return value == value1 || value == value2; } NTCCFG_INLINE bool OpenState::isNot(Value value) const { - return d_value != value; + return d_value.loadAcquire() != value; } NTCCFG_INLINE bool OpenState::isNotEither(Value value1, Value value2) const { - return d_value != value1 && d_value != value2; + const int value = d_value.loadAcquire(); + return value != value1 && value != value2; } } // end namespace ntcs diff --git a/groups/ntc/ntcs/ntcs_shutdownstate.cpp b/groups/ntc/ntcs/ntcs_shutdownstate.cpp index b0e15065..fe0f0539 100644 --- a/groups/ntc/ntcs/ntcs_shutdownstate.cpp +++ b/groups/ntc/ntcs/ntcs_shutdownstate.cpp @@ -25,6 +25,10 @@ namespace ntcs { ShutdownState::ShutdownState() : d_context() +, d_canSend(true) +, d_canReceive(true) +, d_initiated(false) +, d_completed(false) { } @@ -32,6 +36,14 @@ ShutdownState::~ShutdownState() { } +void ShutdownState::refreshAtomicState() +{ + d_canSend.storeRelease(!d_context.shutdownSend()); + d_canReceive.storeRelease(!d_context.shutdownReceive()); + d_initiated.storeRelease(d_context.shutdownInitiated()); + d_completed.storeRelease(d_context.shutdownCompleted()); +} + bool ShutdownState::tryShutdownSend(ntcs::ShutdownContext* context, bool keepHalfOpen) { @@ -66,6 +78,8 @@ bool ShutdownState::tryShutdownSend(ntcs::ShutdownContext* context, } } + this->refreshAtomicState(); + return true; } @@ -106,6 +120,8 @@ bool ShutdownState::tryShutdownReceive(ntcs::ShutdownContext* context, } } + this->refreshAtomicState(); + return true; } @@ -120,11 +136,15 @@ void ShutdownState::close() d_context.setShutdownReceive(true); d_context.setShutdownCompleted(true); + + this->refreshAtomicState(); } void ShutdownState::reset() { d_context.reset(); + + this->refreshAtomicState(); } } // close package namespace diff --git a/groups/ntc/ntcs/ntcs_shutdownstate.h b/groups/ntc/ntcs/ntcs_shutdownstate.h index c06bb2f2..80592c5c 100644 --- a/groups/ntc/ntcs/ntcs_shutdownstate.h +++ b/groups/ntc/ntcs/ntcs_shutdownstate.h @@ -38,17 +38,32 @@ namespace ntcs { /// when sequencing the shutdown of reads and writes for that socket. /// /// @par Thread Safety -/// This class is not thread safe. +/// The observable state accessors ('canSend', 'canReceive', 'initiated', +/// 'completed') read atomic mirrors and may be called concurrently with each +/// other and with a single thread invoking one of the manipulators. Concurrent +/// invocations of the manipulators are not safe. /// /// @ingroup module_ntcs class ShutdownState { ntcs::ShutdownContext d_context; + // The following atomic members mirror the observable predicates derived + // from 'd_context' so that the accessors may be evaluated without holding + // any external lock. They are refreshed from 'd_context' by every + // manipulator (see 'refreshAtomicState'). + bsls::AtomicBool d_canSend; + bsls::AtomicBool d_canReceive; + bsls::AtomicBool d_initiated; + bsls::AtomicBool d_completed; + private: ShutdownState(const ShutdownState&) BSLS_KEYWORD_DELETED; ShutdownState& operator=(const ShutdownState&) BSLS_KEYWORD_DELETED; + /// Refresh the atomic mirrors of the observable state from 'd_context'. + void refreshAtomicState(); + public: /// Create a new object in the default state. ShutdownState(); @@ -124,25 +139,25 @@ class ShutdownState NTCCFG_INLINE bool ShutdownState::initiated() const { - return d_context.shutdownInitiated(); + return d_initiated.loadAcquire(); } NTCCFG_INLINE bool ShutdownState::canSend() const { - return !d_context.shutdownSend(); + return d_canSend.loadAcquire(); } NTCCFG_INLINE bool ShutdownState::canReceive() const { - return !d_context.shutdownReceive(); + return d_canReceive.loadAcquire(); } NTCCFG_INLINE bool ShutdownState::completed() const { - return d_context.shutdownCompleted(); + return d_completed.loadAcquire(); } } // end namespace ntcs From 75a2aa6357b50a3f4bd4038b2733bdb743834bc2 Mon Sep 17 00:00:00 2001 From: Matt Millett Date: Fri, 2 Oct 2026 15:07:20 -0400 Subject: [PATCH 2/2] Take only the send mutex for encrypted sends and merge the state and receive mutexes in stream sockets Continue refining the locking strategy of ntcr::StreamSocket and ntcp::StreamSocket so that sending, including sending encrypted data, is never serialized against receiving, and so that the receive path never needs to release its mutex to perform a cross-cutting transition. Encrypted sends take only 'd_sendMutex' --------------------------------------- Previously 'send' escalated to the state and send mutexes when the socket was encrypted, on the grounds that the encryption session is shared with the receive path. That escalation was unnecessary: * The encryption session is itself thread safe. * 'd_encryption_sp' is written only while holding every mutex, so it is stable while 'd_sendMutex' is held. * Every path that pops outgoing cipher text and enqueues it to the send queue holds 'd_sendMutex', so TLS records are enqueued in the order the session generates them, even when the receive path generates cipher text concurrently (e.g., a TLS 1.3 key update). * Pushing outgoing plain text never invokes the handshake callback (see below), so the encrypted send path never performs a cross-cutting transition. Both 'send' overloads now take only 'd_sendMutex', the 'privateSend' helpers that existed only to support the escalation are folded back into 'send', and 'privateSendEncrypted' again honors 'options.recurse()' as it did before the lock split, removing a forced deferral of send completion. Handshake callback contract --------------------------- Document on ntci::Encryption that implementations invoke the handshake callback only from within 'initiateHandshake' or 'pushIncomingCipherText', i.e., from the call that supplies the input that determines the outcome of the handshake, and never from 'pushOutgoingPlainText', 'popIncomingPlainText', 'popOutgoingCipherText', or 'shutdown'. The stream sockets rely on this guarantee to keep the encrypted send path free of cross-cutting transitions. Both implementations are made to honor it robustly: * ntctls::Session clears the thread's OpenSSL error queue before 'SSL_do_handshake', 'SSL_write', 'SSL_read', and 'SSL_shutdown'. Previously a stale, unrelated error on the calling thread could make 'SSL_get_error' misreport a pending handshake as failed, invoking the handshake callback from an arbitrary call, including on the send path. The failure path also no longer invokes an empty callback if the failure has already been announced. * ntcd::Encryption announces the outcome of the handshake only after all complete incoming frames have been consumed. Previously it released its mutex to invoke the callback while the frame being processed was still in its buffer, so a concurrent caller could observe and re-process that frame and receive a spurious error. The mutex is now handed off to the callback and not re-acquired after it returns, so the callback may safely destroy the encryption session. The stream sockets also retain a reference to the encryption session across 'initiateHandshake' and 'pushIncomingCipherText', since a failed handshake resets 'd_encryption_sp' from within the callback and would otherwise destroy the session while one of its member functions is still executing. 'privateUpgrade' now stops if the callback has already processed the failure of the upgrade, rather than dereferencing the reset pointer. Merge 'd_mutex' into 'd_receiveMutex' ------------------------------------ Each stream socket now defines two mutexes, always acquired in the order 'd_receiveMutex', then 'd_sendMutex': * 'd_receiveMutex' guards the read path and the lifecycle and control state of the socket (the connect and upgrade machinery, and the transitions of the open, shutdown, and detach state machines). Conceptually, it guards the connectedness and potential readability of the socket. * 'd_sendMutex' guards the separate write path. This order follows the flow of data: the read path may feed the write path, so it may acquire 'd_sendMutex' while holding 'd_receiveMutex', but the write path never acquires 'd_receiveMutex' while holding 'd_sendMutex'. No code path acquired the former state mutex and then, separately, the receive mutex, so the merge introduces no self-deadlock. The only operations that now wait behind the read path are opening, binding, connecting, registering resolvers, and the certificate and private key accessors, none of which are on a hot path. Function-level documentation that referred to the removed mutex now names the mutex each function actually requires; several of these contracts were already inaccurate after the original split. Receive path performs cross-cutting transitions in place -------------------------------------------------------- Because the receive path already holds 'd_receiveMutex', it now performs every cross-cutting transition it discovers by additionally acquiring 'd_sendMutex', without releasing 'd_receiveMutex': * Once the handshake has completed, decrypting incoming data requires only 'd_receiveMutex': the handshake callback has been cleared and can no longer be invoked. 'd_upgradeInProgress' selects this path. While the handshake is in progress, the read holds both mutexes, since it may complete or fail the handshake. Encrypted receive callbacks may therefore again be invoked synchronously in the steady state. * Outgoing cipher text generated while decrypting is enqueued by additionally acquiring 'd_sendMutex'. * Receiving the peer's TLS shutdown, or unencrypted leftovers after the encrypted data, completes the downgrade (which resets the encryption session) by additionally acquiring 'd_sendMutex'. The downgrade logic is factored into 'privateReceiveDowngrade', and the cipher text drain into 'privateSendOutgoingCipherText'. * End-of-file and failures are processed by additionally acquiring 'd_sendMutex', replacing the previous release-and-reacquire blocks. Only the write path still releases its mutex and then acquires both mutexes in order when it discovers a cross-cutting transition. In ntcp::StreamSocket, the downgrade is now completed before the data received is announced. Announcing the data may invoke receive callbacks synchronously, which releases 'd_receiveMutex' while they execute; previously a reception initiated concurrently during that window could push unencrypted data into a session that had already received the peer's TLS shutdown, failing the socket. Both stream sockets now order the downgrade events before the data received in the same read. Miscellaneous ------------- The rate-limit announcements in 'privateThrottleSendBuffer' and 'privateThrottleReceiveBuffer' named the former state mutex. They are deferred, so this was latent, but they now name 'd_sendMutex' and 'd_receiveMutex' respectively. Testing ------- ntcr_streamsocket.t, ntcp_streamsocket.t, ntcd_encryption.t, ntcd_test.t, ntctls_plugin.t, the ntcr and ntcp datagram and listener socket drivers, and ntcf_system.t (which exercises TLS upgrade, use, shutdown, and abortive downgrade over ntcr::StreamSocket) pass on Darwin. Darwin provides no proactor driver, so the encrypted paths of ntcp::StreamSocket remain to be exercised by ntcf_system.t on Linux (io_uring) or Windows (I/O completion ports). Co-Authored-By: Claude Opus 5.5 (1M context) --- groups/ntc/ntcd/ntcd_encryption.cpp | 83 ++-- groups/ntc/ntcd/ntcd_encryption.h | 12 +- groups/ntc/ntci/ntci_encryption.h | 33 +- groups/ntc/ntcp/ntcp_streamsocket.cpp | 649 ++++++++++++------------ groups/ntc/ntcp/ntcp_streamsocket.h | 120 +++-- groups/ntc/ntcr/ntcr_streamsocket.cpp | 682 +++++++++++++------------- groups/ntc/ntcr/ntcr_streamsocket.h | 109 ++-- groups/ntc/ntctls/ntctls_plugin.cpp | 28 +- 8 files changed, 914 insertions(+), 802 deletions(-) diff --git a/groups/ntc/ntcd/ntcd_encryption.cpp b/groups/ntc/ntcd/ntcd_encryption.cpp index e2bde306..ef602682 100644 --- a/groups/ntc/ntcd/ntcd_encryption.cpp +++ b/groups/ntc/ntcd/ntcd_encryption.cpp @@ -1366,13 +1366,6 @@ ntsa::Error Encryption::processIncomingHello() d_shutdownState.close(); d_handshakeState = e_FAILED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), - bsl::shared_ptr(), - "Not authorized"); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } else { error = this->enqueueOutgoingAccept(true); @@ -1385,11 +1378,6 @@ ntsa::Error Encryption::processIncomingHello() if (d_role == ntca::EncryptionRole::e_CLIENT) { BSLS_ASSERT(d_remoteCertificate_sp); d_handshakeState = e_ESTABLISHED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(), d_remoteCertificate_sp, ""); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } else { error = this->enqueueOutgoingHello(); @@ -1438,23 +1426,11 @@ ntsa::Error Encryption::processIncomingAccept() if (d_role == ntca::EncryptionRole::e_SERVER) { BSLS_ASSERT(d_remoteCertificate_sp); d_handshakeState = e_ESTABLISHED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(), d_remoteCertificate_sp, ""); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } } else { d_shutdownState.close(); d_handshakeState = e_FAILED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), - bsl::shared_ptr(), - "Not authorized"); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } return ntsa::Error(); @@ -1518,7 +1494,7 @@ ntsa::Error Encryption::processIncomingGoodbye() return ntsa::Error(); } -ntsa::Error Encryption::process() +ntsa::Error Encryption::processData() { ntsa::Error error; @@ -1568,6 +1544,49 @@ ntsa::Error Encryption::process() return ntsa::Error(); } +ntsa::Error Encryption::process(LockGuard* lock) +{ + ntsa::Error error = this->processData(); + + // Announce the outcome of the handshake only after all complete incoming + // frames have been consumed, so that a concurrent caller cannot observe + // (and re-process) a partially-processed frame while the mutex is + // released. The handshake callback is reset when the outcome is + // announced, so the outcome is announced at most once. + + if (d_handshakeCallback && (d_handshakeState == e_ESTABLISHED || + d_handshakeState == e_FAILED)) + { + ntci::Encryption::HandshakeCallback handshakeCallback = + d_handshakeCallback; + + d_handshakeCallback = ntci::Encryption::HandshakeCallback(); + + const bool established = d_handshakeState == e_ESTABLISHED; + + bsl::shared_ptr remoteCertificate; + if (established) { + remoteCertificate = d_remoteCertificate_sp; + } + + // Do not re-acquire the mutex after the handshake callback returns: + // the handshake callback may destroy this object. + + lock->release()->unlock(); + + if (established) { + handshakeCallback(ntsa::Error(), remoteCertificate, ""); + } + else { + handshakeCallback(ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), + remoteCertificate, + "Not authorized"); + } + } + + return error; +} + Encryption::Encryption( ntca::EncryptionRole::Value role, const bsl::shared_ptr& certificate, @@ -1627,7 +1646,7 @@ ntsa::Error Encryption::initiateHandshake(const HandshakeCallback& callback) d_handshakeState = e_HELLO_SENT; } - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushIncomingCipherText(const bdlbb::Blob& input) @@ -1640,7 +1659,7 @@ ntsa::Error Encryption::pushIncomingCipherText(const bdlbb::Blob& input) bdlbb::BlobUtil::append(d_incomingCipherText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushIncomingCipherText(const ntsa::Data& input) @@ -1653,7 +1672,7 @@ ntsa::Error Encryption::pushIncomingCipherText(const ntsa::Data& input) ntsa::DataUtil::append(d_incomingCipherText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushOutgoingPlainText(const bdlbb::Blob& input) @@ -1666,7 +1685,7 @@ ntsa::Error Encryption::pushOutgoingPlainText(const bdlbb::Blob& input) bdlbb::BlobUtil::append(d_outgoingPlainText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushOutgoingPlainText(const ntsa::Data& input) @@ -1679,7 +1698,7 @@ ntsa::Error Encryption::pushOutgoingPlainText(const ntsa::Data& input) ntsa::DataUtil::append(d_outgoingPlainText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::popIncomingPlainText(bdlbb::Blob* output) @@ -1693,7 +1712,7 @@ ntsa::Error Encryption::popIncomingPlainText(bdlbb::Blob* output) d_incomingPlainText_sp->length()); } - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::popOutgoingCipherText(bdlbb::Blob* output) @@ -1707,7 +1726,7 @@ ntsa::Error Encryption::popOutgoingCipherText(bdlbb::Blob* output) d_outgoingCipherText_sp->length()); } - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::shutdown() diff --git a/groups/ntc/ntcd/ntcd_encryption.h b/groups/ntc/ntcd/ntcd_encryption.h index a772db47..cb2f10c3 100644 --- a/groups/ntc/ntcd/ntcd_encryption.h +++ b/groups/ntc/ntcd/ntcd_encryption.h @@ -682,7 +682,17 @@ class Encryption : public ntci::Encryption /// Process the available data through the encryption state machine. /// Return the error. - ntsa::Error process(); + ntsa::Error processData(); + + /// Process the available data through the encryption state machine. + /// If the handshake has completed or failed and that outcome has not + /// yet been announced, release the specified 'lock', unlock the mutex, + /// and invoke the handshake callback. Return the error. Note that + /// the handshake callback is invoked only after all complete incoming + /// frames have been consumed, and the mutex is not re-acquired after the + /// handshake callback returns, so the handshake callback may safely + /// destroy this object. + ntsa::Error process(LockGuard* lock); public: /// Create a new encryption operating in the specified 'role' from an diff --git a/groups/ntc/ntci/ntci_encryption.h b/groups/ntc/ntci/ntci_encryption.h index d64f72f4..05e3e0ed 100644 --- a/groups/ntc/ntci/ntci_encryption.h +++ b/groups/ntc/ntci/ntci_encryption.h @@ -50,6 +50,16 @@ namespace ntci { /// @par Thread Safety /// This class is thread safe. /// +/// @par Handshake Callback Invocation +/// Implementations must invoke the handshake callback only from within a call +/// to 'initiateHandshake' or 'pushIncomingCipherText', i.e., only from within +/// the call that supplies the input that determines the outcome of the +/// handshake. Implementations must not invoke the handshake callback from +/// within any other function, in particular 'pushOutgoingPlainText', +/// 'popIncomingPlainText', 'popOutgoingCipherText', or 'shutdown'. Users of +/// this class rely on this guarantee to call those other functions without +/// being prepared to process the completion or failure of the handshake. +/// /// @ingroup module_ntci_encryption class Encryption { @@ -58,7 +68,9 @@ class Encryption /// a handshake is complete. The first parameter indicates the error, /// if any. The second parameter indicates the certificate of the peer, /// if any. The third parameters indicates the cause of the handshake - /// error, if an error is indicated. + /// error, if an error is indicated. Note that this function is invoked + /// only from within a call to 'initiateHandshake' or + /// 'pushIncomingCipherText'. typedef NTCCFG_FUNCTION( const ntsa::Error& error, const bsl::shared_ptr& certificate, @@ -68,30 +80,37 @@ class Encryption virtual ~Encryption(); /// Initiate the handshake to begin the session. Invoke the specified - /// 'callback' when the handshake completes. Return the error. + /// 'callback' when the handshake completes, either from within this + /// call or from within a subsequent call to 'pushIncomingCipherText'. + /// Return the error. virtual ntsa::Error initiateHandshake(const HandshakeCallback& callback); /// Initiate the handshake to begin the session according to the specified /// 'upgradeOptions'. Invoke the specified 'callback' when the handshake - /// completes. Return the error. + /// completes, either from within this call or from within a subsequent + /// call to 'pushIncomingCipherText'. Return the error. virtual ntsa::Error initiateHandshake( const ntca::UpgradeOptions& upgradeOptions, const HandshakeCallback& callback); /// Add the specified 'input' containing ciphertext read from the peer. - /// Return the error. + /// Invoke the handshake callback if 'input' completes the handshake or + /// causes it to fail. Return the error. virtual ntsa::Error pushIncomingCipherText(const bdlbb::Blob& input); /// Add the specified 'input' containing ciphertext read from the peer. - /// Return the error. + /// Invoke the handshake callback if 'input' completes the handshake or + /// causes it to fail. Return the error. virtual ntsa::Error pushIncomingCipherText(const ntsa::Data& input); /// Add the specified 'input' containing plaintext to be sent to the - /// peer. Return the error. + /// peer. Return the error. Note that this function never invokes the + /// handshake callback. virtual ntsa::Error pushOutgoingPlainText(const bdlbb::Blob& input); /// Add the specified 'input' containing plaintext to be sent to the - /// peer. Return the error. + /// peer. Return the error. Note that this function never invokes the + /// handshake callback. virtual ntsa::Error pushOutgoingPlainText(const ntsa::Data& input); /// Pop plaintext read from the peer and append it to the specified diff --git a/groups/ntc/ntcp/ntcp_streamsocket.cpp b/groups/ntc/ntcp/ntcp_streamsocket.cpp index 45bd04ae..a083d4bb 100644 --- a/groups/ntc/ntcp/ntcp_streamsocket.cpp +++ b/groups/ntc/ntcp/ntcp_streamsocket.cpp @@ -215,11 +215,10 @@ void StreamSocket::processSocketConnected(const ntsa::Error& error) bsl::shared_ptr self = this->getSelf(this); - // Connection completion is cross-cutting; acquire all three mutexes in + // Connection completion is cross-cutting; acquire both mutexes in // canonical order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -254,76 +253,33 @@ void StreamSocket::processSocketReceived(const ntsa::Error& error, return; } - bool encrypted = false; ntsa::Error outcome; - // Fast path: an unencrypted receive completion only needs the receive - // mutex. The encryption object is co-guarded and may be examined under the - // receive mutex; if it is present, escalate to holding all three mutexes in - // canonical order (the encrypted completion decrypts under 'd_mutex' and may - // enqueue outgoing cipher text under 'd_sendMutex'). To avoid a concurrent - // 'privateInitiateReceive' from overwriting 'd_receiveBlob_sp' during the - // lock handoff, 'd_receivePending' is left true until the completion is - // actually processed. - - { - LockGuard receiveLock(&d_receiveMutex); - - if (NTCCFG_UNLIKELY(d_detachState.mode() == - ntcs::DetachMode::e_INITIATED)) - { - return; - } - - if (d_encryption_sp) { - encrypted = true; - } - else { - NTCI_LOG_CONTEXT(); - NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); - NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); - NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); - - d_receivePending = false; + // The receive completion holds the receive mutex throughout. Whenever it + // must perform a cross-cutting transition (completing or failing the + // handshake, downgrading, shutting down, or failing), it additionally + // acquires the send mutex, which the lock order permits, without releasing + // the receive mutex. - if (error) { - if (error != ntsa::Error::e_CANCELLED) { - NTCP_STREAMSOCKET_LOG_RECEIVE_FAILURE(error); - outcome = error; - } - } - else { - NTCP_STREAMSOCKET_LOG_RECEIVE_RESULT(context); - outcome = this->privateCompleteReceive( - self, - context.bytesReceivable(), - context.bytesReceived(), - false); - } + LockGuard receiveLock(&d_receiveMutex); - if (!outcome) { - this->privateInitiateReceive(self); - } - } + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) + { + return; } - if (encrypted) { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); - LockGuard receiveLock(&d_receiveMutex); + NTCI_LOG_CONTEXT(); + NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); + NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); + NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); - if (NTCCFG_UNLIKELY(d_detachState.mode() == - ntcs::DetachMode::e_INITIATED)) - { - return; - } + d_receivePending = false; - NTCI_LOG_CONTEXT(); - NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); - NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); - NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + if (NTCCFG_UNLIKELY(d_encryption_sp && d_upgradeInProgress)) { + // While the handshake is in progress, decrypting may complete or fail + // the handshake, so hold both mutexes while processing the completion. - d_receivePending = false; + LockGuard sendLock(&d_sendMutex); if (error) { if (error != ntsa::Error::e_CANCELLED) { @@ -354,26 +310,39 @@ void StreamSocket::processSocketReceived(const ntsa::Error& error, return; } - // Perform any cross-cutting shutdown or failure sequence for the - // unencrypted path outside the receive mutex, acquiring all three mutexes - // in canonical order. + // Fast path: an unencrypted receive completion, or an encrypted receive + // completion once the handshake has completed, only needs the receive + // mutex. + + if (error) { + if (error != ntsa::Error::e_CANCELLED) { + NTCP_STREAMSOCKET_LOG_RECEIVE_FAILURE(error); + outcome = error; + } + } + else { + NTCP_STREAMSOCKET_LOG_RECEIVE_RESULT(context); + outcome = this->privateCompleteReceive(self, + context.bytesReceivable(), + context.bytesReceived(), + false); + } if (outcome == ntsa::Error::e_EOF) { - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - LockGuard receiveLock(&d_receiveMutex); this->privateShutdownReceive(self, ntsa::ShutdownOrigin::e_REMOTE, true); } else if (outcome) { - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - LockGuard receiveLock(&d_receiveMutex); this->privateFailReceive(self, outcome); } + else { + this->privateInitiateReceive(self); + } } void StreamSocket::processSocketSent(const ntsa::Error& error, @@ -418,11 +387,10 @@ void StreamSocket::processSocketError(const ntsa::Error& error) bsl::shared_ptr self = this->getSelf(this); - // Error handling is cross-cutting (it fails the whole socket); acquire all - // three mutexes in canonical order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); + // Error handling is cross-cutting (it fails the whole socket); acquire + // both mutexes in canonical order. LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -443,11 +411,10 @@ void StreamSocket::processSocketDetached() NTCCFG_OBJECT_GUARD(&d_object); // The detachment completion runs the deferred shutdown/connect-failure - // completion, which is cross-cutting; acquire all three mutexes in + // completion, which is cross-cutting; acquire both mutexes in // canonical order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -473,9 +440,8 @@ void StreamSocket::processConnectDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -517,9 +483,8 @@ void StreamSocket::processConnectRetryTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -557,9 +522,8 @@ void StreamSocket::processUpgradeTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -766,9 +730,13 @@ void StreamSocket::privateEncryptionHandshake( const bsl::string& details) { // IMPLEMENTATION NOTE: This function is always called while holding - // 'd_mutex', 'd_sendMutex', and 'd_receiveMutex' (from the encrypted - // receive completion or from 'privateUpgrade'), so the callback is - // deferred. + // 'd_receiveMutex' and 'd_sendMutex', so the callback is deferred. The + // encryption session invokes this function only from within + // 'initiateHandshake' or 'pushIncomingCipherText' (see 'ntci::Encryption'), + // which are called only from 'privateUpgrade' and the encrypted receive + // completion while the handshake is in progress, each of which holds both + // mutexes. In particular, this function is never called from the + // encrypted send path, which holds only 'd_sendMutex'. NTCI_LOG_CONTEXT(); @@ -799,7 +767,7 @@ void StreamSocket::privateEncryptionHandshake( d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } } else { @@ -950,7 +918,7 @@ void StreamSocket::privateCompleteConnect( NTCI_LOG_TRACE("Connection attempt succeeded"); - // Connection establishment runs while holding all three mutexes, so all + // Connection establishment runs while holding both mutexes, so all // announcements and callbacks are deferred. if (d_session_sp) { @@ -961,7 +929,7 @@ void StreamSocket::privateCompleteConnect( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return; @@ -974,7 +942,7 @@ void StreamSocket::privateCompleteConnect( d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return; @@ -987,7 +955,7 @@ void StreamSocket::privateCompleteConnect( d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } void StreamSocket::privateFailConnect( @@ -1134,7 +1102,7 @@ void StreamSocket::privateFailConnectComplete( { NTCI_LOG_CONTEXT(); - // This cleanup always runs while holding all three mutexes (inline from + // This cleanup always runs while holding both mutexes (inline from // 'privateFailConnect' or from the deferred detachment completion), so all // announcements and callbacks are deferred. NTCCFG_WARNING_UNUSED(defer); @@ -1172,7 +1140,7 @@ void StreamSocket::privateFailConnectComplete( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } if (connectCallback) { @@ -1181,7 +1149,7 @@ void StreamSocket::privateFailConnectComplete( d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } if (d_connectOptions.retryCount().valueOr(bsl::size_t(0)) == 0) { @@ -1201,7 +1169,7 @@ void StreamSocket::privateFailConnectComplete( d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -1256,7 +1224,7 @@ void StreamSocket::privateFailUpgrade( d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } this->privateShutdown(self, @@ -1284,8 +1252,8 @@ void StreamSocket::privateInitiateReceive( ntcs::ObserverRef proactorRef(&d_proactor); if (!proactorRef) { - // Failing the receive is cross-cutting; this function runs holding only - // the receive mutex, so defer the failure to run under all three + // Failing the receive is cross-cutting; this function may run holding + // only the receive mutex, so defer the failure to run under both // mutexes in canonical order. this->execute(NTCCFG_BIND(&StreamSocket::processSocketError, self, @@ -1310,7 +1278,7 @@ void StreamSocket::privateInitiateReceive( error = proactorRef->receive(self, d_receiveBlob_sp.get(), d_receiveOptions); if (error) { - // Failing the receive is cross-cutting; defer it to run under all three + // Failing the receive is cross-cutting; defer it to run under both // mutexes in canonical order. this->execute( NTCCFG_BIND(&StreamSocket::processSocketError, self, error)); @@ -1324,12 +1292,17 @@ ntsa::Error StreamSocket::privateCompleteReceive( const bsl::shared_ptr& self, bsl::size_t numBytesReceivable, bsl::size_t numBytesReceived, - bool defer) + bool exclusive) { NTCI_LOG_CONTEXT(); ntsa::Error error; + // When the caller holds more than one mutex, all announcements and + // callback invocations must be deferred. + + const bool defer = exclusive; + if (numBytesReceived == 0) { // The peer has shut down. Return EOF so the caller // ('processSocketReceived') can perform the cross-cutting shutdown of @@ -1382,7 +1355,14 @@ ntsa::Error StreamSocket::privateCompleteReceive( else { const int receiveQueueLengthBefore = d_receiveQueue.data()->length(); - error = d_encryption_sp->pushIncomingCipherText(*d_receiveBlob_sp); + // Pushing incoming cipher text may invoke the handshake callback, which + // may reset 'd_encryption_sp' if the handshake fails. Retain a + // reference to the encryption session so it is not destroyed while + // one of its member functions is still executing. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->pushIncomingCipherText(*d_receiveBlob_sp); if (NTCCFG_UNLIKELY(error)) { return error; } @@ -1471,6 +1451,54 @@ ntsa::Error StreamSocket::privateCompleteReceive( d_receiveQueue.pushEntry(entry); } + // Complete any downgrade, and enqueue any outgoing cipher text, before + // announcing the data received: announcing the data may invoke receive + // callbacks synchronously, which releases the receive mutex while they + // execute, and no further data must be decrypted after the peer's TLS + // shutdown has been received. + + if (NTCCFG_UNLIKELY(d_encryption_sp)) { + // Receiving the peer's TLS shutdown, or unencrypted leftovers after + // the encrypted data, downgrades the connection, which resets the + // encryption session: a cross-cutting transition that requires both + // mutexes. The lock order permits acquiring 'd_sendMutex' while + // holding 'd_receiveMutex'. + + if (NTCCFG_UNLIKELY(downgradeAbortively || + d_encryption_sp->isShutdownReceived())) + { + if (exclusive) { + error = this->privateReceiveDowngrade(self, + downgradeAbortively); + } + else { + LockGuard sendLock(&d_sendMutex); + error = this->privateReceiveDowngrade(self, + downgradeAbortively); + } + + if (error) { + return error; + } + } + else if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + // Enqueue any outgoing cipher text generated while decrypting, + // e.g., the response to a TLS 1.3 key update. + + if (exclusive) { + error = this->privateSendOutgoingCipherText(self); + } + else { + LockGuard sendLock(&d_sendMutex); + error = this->privateSendOutgoingCipherText(self); + } + + if (error) { + return error; + } + } + } + NTCP_STREAMSOCKET_LOG_READ_QUEUE_FILLED(d_receiveQueue.size()); NTCS_METRICS_UPDATE_READ_QUEUE_SIZE(d_receiveQueue.size()); @@ -1606,9 +1634,67 @@ ntsa::Error StreamSocket::privateCompleteReceive( } } - if (NTCCFG_UNLIKELY(d_encryption_sp)) { - if (NTCCFG_UNLIKELY(downgradeAbortively)) { + return ntsa::Error(); +} + +ntsa::Error StreamSocket::privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive) +{ + ntsa::Error error; + + if (!d_encryption_sp) { + return ntsa::Error(); + } + + if (NTCCFG_UNLIKELY(abortive)) { + if (!d_encryption_sp->isShutdownSent()) { + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_INITIATED); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeInitiated( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_proactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + + d_encryption_sp.reset(); + + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_COMPLETE); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeComplete(d_session_sp, + self, + event, + d_sessionStrand_sp, + d_proactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + else { + if (d_encryption_sp->isShutdownReceived()) { if (!d_encryption_sp->isShutdownSent()) { + error = d_encryption_sp->shutdown(); + if (error) { + return error; + } + if (d_session_sp) { ntca::DowngradeContext context; @@ -1623,11 +1709,18 @@ ntsa::Error StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - defer, - &d_mutex); + true, + &d_receiveMutex); } } + } + + error = this->privateSendOutgoingCipherText(self); + if (error) { + return error; + } + if (d_encryption_sp->isShutdownFinished()) { d_encryption_sp.reset(); if (d_session_sp) { @@ -1637,90 +1730,44 @@ ntsa::Error StreamSocket::privateCompleteReceive( event.setType(ntca::DowngradeEventType::e_COMPLETE); event.setContext(context); - ntcs::Dispatch::announceDowngradeComplete(d_session_sp, - self, - event, - d_sessionStrand_sp, - d_proactorStrand_sp, - self, - defer, - &d_mutex); + ntcs::Dispatch::announceDowngradeComplete( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_proactorStrand_sp, + self, + true, + &d_receiveMutex); } } - else { - if (d_encryption_sp->isShutdownReceived()) { - if (!d_encryption_sp->isShutdownSent()) { - error = d_encryption_sp->shutdown(); - if (error) { - return error; - } - - if (d_session_sp) { - ntca::DowngradeContext context; - - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_INITIATED); - event.setContext(context); - - ntcs::Dispatch::announceDowngradeInitiated( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_proactorStrand_sp, - self, - defer, - &d_mutex); - } - } - } - - if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { - bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); - - while ( - NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) - { - error = - d_encryption_sp->popOutgoingCipherText(&cipherData); - if (error) { - return error; - } - } + } - if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { - error = this->privateSendRaw(self, - cipherData, - ntca::SendOptions(), - ntca::SendContext(), - d_sendComplete); - if (error) { - return error; - } - } - } + return ntsa::Error(); +} - if (d_encryption_sp->isShutdownFinished()) { - d_encryption_sp.reset(); +ntsa::Error StreamSocket::privateSendOutgoingCipherText( + const bsl::shared_ptr& self) +{ + ntsa::Error error; - if (d_session_sp) { - ntca::DowngradeContext context; + bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_COMPLETE); - event.setContext(context); + while (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + error = d_encryption_sp->popOutgoingCipherText(&cipherData); + if (error) { + return error; + } + } - ntcs::Dispatch::announceDowngradeComplete( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_proactorStrand_sp, - self, - defer, - &d_mutex); - } - } + if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { + error = this->privateSendRaw(self, + cipherData, + ntca::SendOptions(), + ntca::SendContext(), + d_sendComplete); + if (error) { + return error; } } @@ -1817,7 +1864,7 @@ void StreamSocket::privateInitiateSend( else { // A graceful shutdown sentinel has been drained. Shutting down for // sending is cross-cutting; this function runs holding only the - // send mutex, so defer the shutdown to run under all three mutexes + // send mutex, so defer the shutdown to run under both mutexes // in canonical order (via the public 'shutdown', which is a no-op // sentinel push once the write queue is empty). d_sendQueue.popEntry(); @@ -1993,7 +2040,7 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } this->privateShutdown(self, @@ -2058,7 +2105,7 @@ ntsa::Error StreamSocket::privateShutdown( d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2219,7 +2266,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2290,7 +2337,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2310,7 +2357,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2330,7 +2377,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2346,7 +2393,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2385,7 +2432,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } callbackEntryVector.clear(); @@ -2405,7 +2452,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } if (d_session_sp) { @@ -2420,7 +2467,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2437,7 +2484,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } // Note that detachment from the proactor is handled earlier in this @@ -2480,13 +2527,13 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); if (d_closeCallback) { d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2789,7 +2836,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -2858,7 +2905,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -3037,13 +3084,13 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so - // the completion callback is always deferred. + const bool defer = !options.recurse(); + callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - true, + defer, &d_sendMutex); } @@ -3085,13 +3132,13 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so - // the completion callback is always deferred. + const bool defer = !options.recurse(); + callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - true, + defer, &d_sendMutex); } @@ -3387,7 +3434,7 @@ ntsa::Error StreamSocket::privateOpen( d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -3404,7 +3451,7 @@ void StreamSocket::processSourceEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; ntca::BindContext bindContext; @@ -3464,7 +3511,7 @@ void StreamSocket::processSourceEndpointResolution( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -3480,9 +3527,8 @@ void StreamSocket::processRemoteEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -3585,7 +3631,7 @@ void StreamSocket::processRemoteEndpointResolution( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) @@ -3625,12 +3671,23 @@ ntsa::Error StreamSocket::privateUpgrade( bdlf::MemFnUtil::memFn(&StreamSocket::privateEncryptionHandshake, this); - error = - d_encryption_sp->initiateHandshake(upgradeOptions, handshakeCallback); + // Initiating the handshake and pushing incoming cipher text may invoke + // the handshake callback, which resets 'd_encryption_sp' if the handshake + // fails. Retain a reference to the encryption session so it is not + // destroyed while one of its member functions is still executing, and + // stop if the failure of the upgrade has already been processed. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->initiateHandshake(upgradeOptions, handshakeCallback); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + // Push any unconsumed receive queue data that must be now considered // cipher text into the encryption session. @@ -3638,12 +3695,15 @@ ntsa::Error StreamSocket::privateUpgrade( // Push the receive queue data into the encryption session as incoming // cipher text. - error = - d_encryption_sp->pushIncomingCipherText(*d_receiveQueue.data()); + error = encryption->pushIncomingCipherText(*d_receiveQueue.data()); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + bdlbb::BlobUtil::erase(d_receiveQueue.data().get(), 0, d_receiveQueue.data()->length()); @@ -3757,7 +3817,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -3798,7 +3858,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -3858,7 +3918,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } d_encryption_sp.reset(); @@ -3877,7 +3937,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -4089,7 +4149,7 @@ ntsa::Error StreamSocket::privateRetryConnectToEndpoint( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -4140,7 +4200,6 @@ StreamSocket::StreamSocket( const bsl::shared_ptr& metrics, bslma::Allocator* basicAllocator) : d_object("ntcp::StreamSocket") -, d_mutex() , d_sendMutex() , d_receiveMutex() , d_transport(ntsa::Transport::e_UNDEFINED) @@ -4312,7 +4371,7 @@ ntsa::Error StreamSocket::open() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self); } @@ -4321,7 +4380,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport); } @@ -4331,7 +4390,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle); } @@ -4342,7 +4401,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket); } @@ -4354,7 +4413,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle, acceptor); } @@ -4366,7 +4425,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket, acceptor); } @@ -4386,7 +4445,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4426,7 +4485,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -4455,7 +4514,7 @@ ntsa::Error StreamSocket::bind(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4502,7 +4561,7 @@ ntsa::Error StreamSocket::connect(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4648,7 +4707,7 @@ ntsa::Error StreamSocket::connect(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4773,9 +4832,8 @@ ntsa::Error StreamSocket::upgrade( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -4933,29 +4991,17 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, { bsl::shared_ptr self = this->getSelf(this); - // Fast path: an unencrypted send only needs the send mutex. The encryption - // object is co-guarded and may be examined under the send mutex; if it is - // present, escalate to also holding the state mutex (in canonical order), - // since the encrypted send path operates on the shared encryption object. - - { - LockGuard sendLock(&d_sendMutex); - if (NTCCFG_LIKELY(!d_encryption_sp)) { - return this->privateSend(self, data, options, callback); - } - } + // Both unencrypted and encrypted sends need only the send mutex. The + // encryption object is co-guarded, so it is stable while the send mutex is + // held, and it is itself thread safe. Every path that pops outgoing + // cipher text and enqueues it to the send queue holds the send mutex, so + // TLS records are enqueued in the order they are generated. Pushing + // outgoing plain text never invokes the handshake callback (see + // 'ntci::Encryption'), so the encrypted send path never performs a + // cross-cutting transition. - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - return this->privateSend(self, data, options, callback); -} -ntsa::Error StreamSocket::privateSend( - const bsl::shared_ptr& self, - const bdlbb::Blob& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback) -{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -5074,27 +5120,11 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, { bsl::shared_ptr self = this->getSelf(this); - // Fast path: an unencrypted send only needs the send mutex; escalate to - // also hold the state mutex (in canonical order) only when encrypted. - - { - LockGuard sendLock(&d_sendMutex); - if (NTCCFG_LIKELY(!d_encryption_sp)) { - return this->privateSend(self, data, options, callback); - } - } + // Both unencrypted and encrypted sends need only the send mutex. See the + // 'bdlbb::Blob' overload of this function. - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - return this->privateSend(self, data, options, callback); -} -ntsa::Error StreamSocket::privateSend( - const bsl::shared_ptr& self, - const ntsa::Data& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback) -{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -5490,14 +5520,14 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntsa::Error StreamSocket::registerResolver( const bsl::shared_ptr& resolver) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver = resolver; return ntsa::Error(); } ntsa::Error StreamSocket::deregisterResolver() { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver.reset(); return ntsa::Error(); } @@ -5507,9 +5537,8 @@ ntsa::Error StreamSocket::registerManager( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (manager) { d_manager_sp = manager; @@ -5529,9 +5558,8 @@ ntsa::Error StreamSocket::registerManager( ntsa::Error StreamSocket::deregisterManager() { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_manager_sp.reset(); d_managerStrand_sp.reset(); @@ -5544,9 +5572,8 @@ ntsa::Error StreamSocket::registerSession( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (session) { d_session_sp = session; @@ -5576,9 +5603,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -5615,9 +5641,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -5647,9 +5672,8 @@ ntsa::Error StreamSocket::registerSessionCallback( ntsa::Error StreamSocket::deregisterSession() { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_session_sp.reset(); d_sessionStrand_sp.reset(); @@ -5737,7 +5761,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5776,7 +5800,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5817,7 +5841,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5839,7 +5863,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5920,7 +5944,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5991,9 +6015,8 @@ ntsa::Error StreamSocket::relaxFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6010,9 +6033,8 @@ ntsa::Error StreamSocket::applyFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6054,9 +6076,8 @@ ntsa::Error StreamSocket::cancel(const ntca::ConnectToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6082,9 +6103,8 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6118,7 +6138,7 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } this->privateFail(self, ntsa::Error(ntsa::Error::e_CANCELLED)); @@ -6167,7 +6187,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -6208,7 +6228,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -6220,9 +6240,8 @@ ntsa::Error StreamSocket::downgrade() { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); return this->privateDowngrade(self, ntca::DowngradeOptions()); } @@ -6231,9 +6250,8 @@ ntsa::Error StreamSocket::downgrade(const ntca::DowngradeOptions& options) { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (options.abortive().isNull() || !options.abortive().value()) { return this->privateDowngrade(self, options); @@ -6248,9 +6266,8 @@ ntsa::Error StreamSocket::shutdown(ntsa::ShutdownType::Value direction, { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6299,9 +6316,8 @@ ntsa::Error StreamSocket::release(ntsa::Handle* result, { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); *result = ntsa::k_INVALID_HANDLE; @@ -6337,9 +6353,8 @@ void StreamSocket::close(const ntci::CloseCallback& callback) { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); this->privateClose(self, callback); } @@ -6460,20 +6475,20 @@ ntsa::Transport::Value StreamSocket::transport() const ntsa::Endpoint StreamSocket::sourceEndpoint() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_systemSourceEndpoint; } ntsa::Endpoint StreamSocket::remoteEndpoint() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_systemRemoteEndpoint; } bsl::shared_ptr StreamSocket::sourceCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -6486,7 +6501,7 @@ bsl::shared_ptr StreamSocket::sourceCertificate() bsl::shared_ptr StreamSocket::remoteCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -6498,7 +6513,7 @@ bsl::shared_ptr StreamSocket::remoteCertificate() bsl::shared_ptr StreamSocket::privateKey() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { diff --git a/groups/ntc/ntcp/ntcp_streamsocket.h b/groups/ntc/ntcp/ntcp_streamsocket.h index ab5221e4..08b88cbe 100644 --- a/groups/ntc/ntcp/ntcp_streamsocket.h +++ b/groups/ntc/ntcp/ntcp_streamsocket.h @@ -90,30 +90,47 @@ class StreamSocket : public ntci::StreamSocket, ntccfg::Object d_object; // LOCKING STRATEGY // - // 'd_mutex' (the "state" mutex) guards the shared lifecycle and control - // state of the socket: the connect/upgrade machinery, the open, shutdown, - // and detach state machines' *transitions*, and the encryption object. + // 'd_receiveMutex' guards the read path (the receive queue and incoming + // completion state) and the shared lifecycle and control state of the + // socket: the connect/upgrade machinery and the open, shutdown, and detach + // state machines' *transitions*. Conceptually, it guards the + // connectedness and potential readability of the socket. 'd_sendMutex' + // guards the separate write path (the send queue and outgoing completion + // state), so that sending is not serialized against receiving. // - // 'd_sendMutex' guards the write path (the send queue and outgoing - // completion state) and 'd_receiveMutex' guards the read path (the receive - // queue and incoming completion state). These two leaf mutexes are - // independent so that sending is not serialized against receiving. + // The encryption object is itself thread safe. Encrypting outgoing data + // requires only 'd_sendMutex': every path that pops outgoing cipher text + // and enqueues it to the send queue holds 'd_sendMutex', so TLS records + // are enqueued in the order they are generated, and pushing outgoing plain + // text never invokes the handshake callback. // - // When more than one is held they are always acquired in the order - // 'd_mutex' -> 'd_sendMutex' -> 'd_receiveMutex'. A steady-state I/O - // completion holds only its own leaf; when it must perform a cross-cutting - // lifecycle transition (connection establishment, failure, shutdown) it - // first releases its leaf, then re-acquires in canonical order (or defers - // the transition via 'execute'). + // Decrypting incoming data in the steady state (i.e., once the handshake + // has completed) requires only 'd_receiveMutex': the handshake callback + // can no longer be invoked. Decrypting incoming data while the handshake + // is in progress holds both mutexes, since it may complete or fail the + // handshake (a cross-cutting transition). + // + // When both are held they are always acquired in the order + // 'd_receiveMutex' -> 'd_sendMutex'. This order follows the flow of data: + // the read path may feed the write path, so it may acquire 'd_sendMutex' + // while holding 'd_receiveMutex', but the write path never acquires + // 'd_receiveMutex' while holding 'd_sendMutex'. Consequently, the receive + // completion performs every cross-cutting transition it discovers + // (enqueueing outgoing cipher text generated while decrypting, receiving + // the peer's TLS shutdown, end-of-file, or failure) in place, by + // additionally acquiring 'd_sendMutex' without releasing + // 'd_receiveMutex'. The write path holds only 'd_sendMutex'; when it must + // perform a cross-cutting lifecycle transition (connection establishment, + // failure, shutdown) it first releases 'd_sendMutex', then acquires both + // in canonical order (or defers the transition via 'execute'). // // The observable lifecycle predicates ('d_openState', 'd_shutdownState', // 'd_detachState.mode()', 'd_connectInProgress') are atomic and may be read // without any lock. 'd_flowControlState' is itself thread safe (lock-free). // The co-guarded members ('d_socket_sp', 'd_encryption_sp', 'd_session_sp', // 'd_sessionStrand_sp', 'd_manager_sp', 'd_managerStrand_sp') are written - // only while holding all three mutexes and may therefore be read while - // holding any one of them. - mutable Mutex d_mutex; + // only while holding both mutexes and may therefore be read while holding + // either one of them. mutable Mutex d_sendMutex; mutable Mutex d_receiveMutex; ntsa::Transport::Value d_transport; @@ -259,7 +276,7 @@ class StreamSocket : public ntci::StreamSocket, const bsl::string& details); /// Process the completion of the connection attempt. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless both 'd_receiveMutex' and 'd_sendMutex' are locked. void privateCompleteConnect(const bsl::shared_ptr& self); /// Indicate a connection failure has occurred. If the specified 'defer' @@ -286,7 +303,7 @@ class StreamSocket : public ntci::StreamSocket, bool defer); /// Process the failure of the upgrade attempt. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless both 'd_receiveMutex' and 'd_sendMutex' are locked. void privateFailUpgrade(const bsl::shared_ptr& self, const ntsa::Error& error, const bsl::string& errorDescription); @@ -296,28 +313,45 @@ class StreamSocket : public ntci::StreamSocket, /// Process the completion of the reception of raw or encrypted data /// according to the specified 'numBytesReceivable' and 'numBytesReceived'. - /// If the specified 'defer' flag is true, force all event announcements and - /// callback invocations made during processing to be deferred (used when - /// running while holding more than one mutex, e.g. the encrypted read - /// path). Return 'ntsa::Error::e_EOF' if the peer has shut down, another - /// error if the reception failed, or a default-constructed error on - /// success; the caller performs any resulting shutdown or failure sequence - /// after releasing the receive mutex. The behavior is undefined unless - /// 'd_receiveMutex' is locked. + /// If the specified 'exclusive' flag is true, the caller holds both + /// 'd_receiveMutex' and 'd_sendMutex', and all event announcements and + /// callback invocations made during processing are deferred. Otherwise, + /// the caller holds only 'd_receiveMutex', 'd_sendMutex' is additionally + /// acquired as necessary, and the behavior is undefined unless the + /// encryption handshake, if any, has completed. Return + /// 'ntsa::Error::e_EOF' if the peer has shut down, another error if the + /// reception failed, or a default-constructed error on success; the + /// caller performs any resulting shutdown or failure sequence. ntsa::Error privateCompleteReceive( const bsl::shared_ptr& self, bsl::size_t numBytesReceivable, bsl::size_t numBytesReceived, - bool defer); + bool exclusive); + + /// Complete the downgrade from encrypted to unencrypted communication + /// initiated by the receipt of the peer's TLS shutdown, or, if the + /// specified 'abortive' flag is true, by the receipt of unencrypted + /// leftovers after the encrypted data. Enqueue any outgoing cipher text + /// generated as a result. The behavior is undefined unless both + /// 'd_receiveMutex' and 'd_sendMutex' are locked. Return the error. + ntsa::Error privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive); + + /// Pop all outgoing cipher text from the encryption session and enqueue + /// it for transmission. The behavior is undefined unless 'd_sendMutex' is + /// locked. Return the error. + ntsa::Error privateSendOutgoingCipherText( + const bsl::shared_ptr& self); /// Process the completion of the reception of the raw or unencrypted /// data according to the specified 'numBytesReceived'. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless 'd_receiveMutex' is locked. void privateCompleteReceiveRaw(const bsl::shared_ptr& self, bsl::size_t numBytesReceived); /// Process the failure of the reception of a message. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless both 'd_receiveMutex' and 'd_sendMutex' are locked. void privateFailReceive(const bsl::shared_ptr& self, const ntsa::Error& error); @@ -328,7 +362,7 @@ class StreamSocket : public ntci::StreamSocket, /// Process the completion of the transmission of raw or /// already-encrypted data at the head of the write queue according to /// the specified 'numBytesSent'. The behavior is undefined unless - /// 'd_mutex' is locked. + /// 'd_sendMutex' is locked. void privateCompleteSend(const bsl::shared_ptr& self, bsl::size_t numBytesSent); @@ -336,7 +370,7 @@ class StreamSocket : public ntci::StreamSocket, /// of the write queue. Announce the failure of the head of the write /// queue if a callback is associated with the head of the write queue /// then pop the head of the write queue. The behavior is undefined - /// unless 'd_mutex' is locked. + /// unless 'd_sendMutex' is locked. void privateFailSend(const bsl::shared_ptr& self, const ntsa::Error& error); @@ -388,7 +422,8 @@ class StreamSocket : public ntci::StreamSocket, bool defer); /// Enable copying from the socket buffers in the specified 'direction'. - /// The behavior is undefined unless 'd_mutex' is locked. + /// The behavior is undefined unless the mutex guarding the 'direction' + /// (or both mutexes, for 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateRelaxFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, @@ -397,7 +432,8 @@ class StreamSocket : public ntci::StreamSocket, /// Disable copying from socket buffers in the specified 'direction' /// according to the specified 'mode'. The behavior is undefined unless - /// 'd_mutex' is locked. + /// the mutex guarding the 'direction' (or both mutexes, for + /// 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateApplyFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, @@ -429,24 +465,6 @@ class StreamSocket : public ntci::StreamSocket, ntsa::Error privateThrottleReceiveBuffer( const bsl::shared_ptr& self); - /// Enqueue the specified 'data' for transmission according to the - /// specified 'options', invoking the specified 'callback' on completion. - /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the - /// socket is encrypted, 'd_mutex' as well). Return the error. - ntsa::Error privateSend(const bsl::shared_ptr& self, - const bdlbb::Blob& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback); - - /// Enqueue the specified 'data' for transmission according to the - /// specified 'options', invoking the specified 'callback' on completion. - /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the - /// socket is encrypted, 'd_mutex' as well). Return the error. - ntsa::Error privateSend(const bsl::shared_ptr& self, - const ntsa::Data& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback); - /// Send the specified raw or already encrypted 'data' according to the /// specified 'options'. When the 'data' is entirely copied to the /// send buffer, invoke the specified 'callback' on callback's strand. diff --git a/groups/ntc/ntcr/ntcr_streamsocket.cpp b/groups/ntc/ntcr/ntcr_streamsocket.cpp index 7efb91b3..3ee7b55a 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.cpp +++ b/groups/ntc/ntcr/ntcr_streamsocket.cpp @@ -306,76 +306,29 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) } ntsa::Error error; - bool encrypted = false; - // Fast path: an unencrypted read only needs the receive mutex. The - // encryption object is co-guarded and may be examined while holding the - // receive mutex; if it is present, escalate to holding all three mutexes in - // canonical order (the encrypted read path decrypts under 'd_mutex' and may - // enqueue outgoing cipher text under 'd_sendMutex'). + // The read path holds the receive mutex throughout. Whenever it must + // perform a cross-cutting transition (completing or failing the + // handshake, downgrading, shutting down, or failing), it additionally + // acquires the send mutex, which the lock order permits, without releasing + // the receive mutex. - { - LockGuard receiveLock(&d_receiveMutex); - - if (NTCCFG_UNLIKELY(d_detachState.mode() == - ntcs::DetachMode::e_INITIATED)) - { - return; - } - - if (!d_shutdownState.canReceive()) { - return; - } - - if (d_encryption_sp) { - encrypted = true; - } - else { - bsl::size_t numIterations = 0; - - while (true) { - ++numIterations; - - error = this->privateSocketReadableIteration(self, false); - if (error) { - break; - } - - if (!d_receiveGreedily) { - break; - } - - if (!d_shutdownState.canReceive()) { - break; - } - } + LockGuard receiveLock(&d_receiveMutex); - if (numIterations > 0) { - NTCS_METRICS_UPDATE_RECEIVE_ITERATIONS(numIterations); - } + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) + { + return; + } - if (!(error && error != ntsa::Error::e_WOULD_BLOCK) && - error != ntsa::Error::e_EOF) - { - this->privateRearmAfterReceive(self); - } - } + if (!d_shutdownState.canReceive()) { + return; } - if (encrypted) { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); - LockGuard receiveLock(&d_receiveMutex); + if (NTCCFG_UNLIKELY(d_encryption_sp && d_upgradeInProgress)) { + // While the handshake is in progress, decrypting may complete or fail + // the handshake, so hold both mutexes for the entire read. - if (NTCCFG_UNLIKELY(d_detachState.mode() == - ntcs::DetachMode::e_INITIATED)) - { - return; - } - - if (!d_shutdownState.canReceive()) { - return; - } + LockGuard sendLock(&d_sendMutex); bsl::size_t numIterations = 0; @@ -416,13 +369,34 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) return; } - // Perform any cross-cutting shutdown or failure sequence outside the - // receive mutex, acquiring all three mutexes in canonical order. + // Fast path: an unencrypted read, or an encrypted read once the handshake + // has completed, only needs the receive mutex. + + bsl::size_t numIterations = 0; + + while (true) { + ++numIterations; + + error = this->privateSocketReadableIteration(self, false); + if (error) { + break; + } + + if (!d_receiveGreedily) { + break; + } + + if (!d_shutdownState.canReceive()) { + break; + } + } + + if (numIterations > 0) { + NTCS_METRICS_UPDATE_RECEIVE_ITERATIONS(numIterations); + } if (error == ntsa::Error::e_EOF) { - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - LockGuard receiveLock(&d_receiveMutex); this->privateShutdownReceive(self, ntsa::ShutdownOrigin::e_REMOTE, @@ -430,12 +404,13 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) this->privateFail(self, error); } else if (error && error != ntsa::Error::e_WOULD_BLOCK) { - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - LockGuard receiveLock(&d_receiveMutex); this->privateFail(self, error); } + else { + this->privateRearmAfterReceive(self); + } } void StreamSocket::processSocketWritable(const ntca::ReactorEvent& event) @@ -462,11 +437,10 @@ void StreamSocket::processSocketWritable(const ntca::ReactorEvent& event) if (NTCCFG_UNLIKELY(d_connectInProgress)) { // Connection establishment is a cross-cutting transition (it sets up // both the send and receive paths and announces establishment); - // acquire all three mutexes in canonical order. + // acquire both mutexes in canonical order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) @@ -537,19 +511,17 @@ void StreamSocket::processSocketWritable(const ntca::ReactorEvent& event) } // Perform any cross-cutting failure or shutdown sequence outside the send - // mutex, acquiring all three mutexes in canonical order. + // mutex, acquiring both mutexes in canonical order. if (error && error != ntsa::Error::e_WOULD_BLOCK) { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); this->privateFail(self, error); } else if (shutdownSendPending) { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); this->privateShutdownSend(self, true); } @@ -562,11 +534,10 @@ void StreamSocket::processSocketError(const ntca::ReactorEvent& event) bsl::shared_ptr self = this->getSelf(this); // Error handling is cross-cutting (it may fail a connect, an upgrade, or - // the established connection); acquire all three mutexes in canonical + // the established connection); acquire both mutexes in canonical // order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -638,11 +609,10 @@ void StreamSocket::processConnectDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - // Connect failure is cross-cutting; acquire all three mutexes in canonical + // Connect failure is cross-cutting; acquire both mutexes in canonical // order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -684,11 +654,10 @@ void StreamSocket::processConnectRetryTimer( bsl::shared_ptr self = this->getSelf(this); - // Connect retry/failure is cross-cutting; acquire all three mutexes in + // Connect retry/failure is cross-cutting; acquire both mutexes in // canonical order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -726,11 +695,10 @@ void StreamSocket::processUpgradeTimer( bsl::shared_ptr self = this->getSelf(this); - // Upgrade failure is cross-cutting; acquire all three mutexes in canonical + // Upgrade failure is cross-cutting; acquire both mutexes in canonical // order. - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -935,8 +903,13 @@ void StreamSocket::privateEncryptionHandshake( const bsl::string& details) { // IMPLEMENTATION NOTE: This function is always called while holding - // 'd_mutex', 'd_sendMutex', and 'd_receiveMutex' (either from the encrypted - // read path or from 'privateUpgrade'), so all callbacks are deferred. + // 'd_receiveMutex' and 'd_sendMutex', so all callbacks are deferred. The + // encryption session invokes this function only from within + // 'initiateHandshake' or 'pushIncomingCipherText' (see 'ntci::Encryption'), + // which are called only from 'privateUpgrade' and the encrypted read path + // while the handshake is in progress, each of which holds both mutexes. + // In particular, this function is never called from the encrypted send + // path, which holds only 'd_sendMutex'. NTCI_LOG_CONTEXT(); @@ -967,7 +940,7 @@ void StreamSocket::privateEncryptionHandshake( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } } else { @@ -979,12 +952,17 @@ void StreamSocket::privateEncryptionHandshake( ntsa::Error StreamSocket::privateSocketReadableIteration( const bsl::shared_ptr& self, - bool defer) + bool exclusive) { NTCI_LOG_CONTEXT(); ntsa::Error error; + // When the caller holds more than one mutex, all announcements and + // callback invocations must be deferred. + + const bool defer = exclusive; + if (d_receiveQueue.isHighWatermarkViolated()) { return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); } @@ -992,7 +970,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( ntsa::ReceiveContext context; error = this->privateDequeueReceiveBuffer(self, &context, - d_receiveQueue.data().get()); + d_receiveQueue.data().get(), + exclusive); if (NTCCFG_UNLIKELY(error)) { return error; } @@ -1315,7 +1294,7 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( NTCI_LOG_TRACE("Connection attempt succeeded"); - // Connection establishment runs while holding all three mutexes, so all + // Connection establishment runs while holding both mutexes, so all // announcements and callbacks are deferred. if (d_session_sp) { @@ -1326,7 +1305,7 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return ntsa::Error(ntsa::Error::e_CONNECTION_DEAD); @@ -1339,7 +1318,7 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return ntsa::Error(ntsa::Error::e_CONNECTION_DEAD); @@ -1352,7 +1331,7 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -1819,11 +1798,10 @@ void StreamSocket::privateFailConnectPart2( if (lock) { // Invoked asynchronously as the reactor detachment callback. Acquire - // all three mutexes in canonical order because connect failure cleanup + // both mutexes in canonical order because connect failure cleanup // touches the send and receive paths and the shared state. - d_mutex.lock(); - d_sendMutex.lock(); d_receiveMutex.lock(); + d_sendMutex.lock(); BSLS_ASSERT(d_detachState.mode() == ntcs::DetachMode::e_INITIATED); d_detachState.setMode(ntcs::DetachMode::e_IDLE); } @@ -1858,7 +1836,7 @@ void StreamSocket::privateFailConnectPart2( NTCCFG_WARNING_UNUSED(defer); - // This cleanup runs while holding all three mutexes, so all announcements + // This cleanup runs while holding both mutexes, so all announcements // and callbacks are forced to be deferred. if (d_session_sp) { @@ -1869,7 +1847,7 @@ void StreamSocket::privateFailConnectPart2( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } if (connectCallback) { @@ -1878,14 +1856,14 @@ void StreamSocket::privateFailConnectPart2( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } if (d_closeCallback) { d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -1913,9 +1891,8 @@ void StreamSocket::privateFailConnectPart2( d_deferredCalls.clear(); if (lock) { - d_receiveMutex.unlock(); d_sendMutex.unlock(); - d_mutex.unlock(); + d_receiveMutex.unlock(); } } @@ -1964,7 +1941,7 @@ void StreamSocket::privateFailUpgrade( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } this->privateShutdown(self, @@ -2032,7 +2009,7 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } this->privateShutdown(self, @@ -2097,7 +2074,7 @@ ntsa::Error StreamSocket::privateShutdown( d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2227,11 +2204,10 @@ void StreamSocket::privateShutdownSequenceComplete( if (lock) { // Invoked asynchronously as the reactor detachment callback. Acquire - // all three mutexes in canonical order because the shutdown completion + // both mutexes in canonical order because the shutdown completion // touches both the send and receive paths as well as the shared state. - d_mutex.lock(); - d_sendMutex.lock(); d_receiveMutex.lock(); + d_sendMutex.lock(); BSLS_ASSERT(d_detachState.mode() == ntcs::DetachMode::e_INITIATED); d_detachState.setMode(ntcs::DetachMode::e_IDLE); } @@ -2276,7 +2252,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2360,7 +2336,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2380,7 +2356,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2399,7 +2375,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2415,7 +2391,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2454,7 +2430,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } callbackEntryVector.clear(); @@ -2474,7 +2450,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } if (d_session_sp) { @@ -2489,7 +2465,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2506,7 +2482,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } // Note that detachment from the reactor is handled earlier in this @@ -2549,13 +2525,13 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); if (d_closeCallback) { d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2575,9 +2551,8 @@ void StreamSocket::privateShutdownSequenceComplete( d_deferredCalls.clear(); if (lock) { - d_receiveMutex.unlock(); d_sendMutex.unlock(); - d_mutex.unlock(); + d_receiveMutex.unlock(); } } @@ -2782,8 +2757,8 @@ bool StreamSocket::privateCloseFlowControl( const ntci::SocketDetachedCallback& detachCallback) { // This function is only invoked from the cross-cutting shutdown sequence, - // which holds 'd_mutex', 'd_sendMutex', and 'd_receiveMutex'. Both - // direction-scoped flow control states are therefore closed here. + // which holds 'd_receiveMutex' and 'd_sendMutex'. Both directions of flow + // control are therefore closed here. { ntcs::FlowControlContext context; @@ -2913,7 +2888,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -2982,7 +2957,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -3219,7 +3194,8 @@ ntsa::Error StreamSocket::privateEnqueueSendBuffer( ntsa::Error StreamSocket::privateDequeueReceiveBuffer( const bsl::shared_ptr& self, ntsa::ReceiveContext* context, - bdlbb::Blob* data) + bdlbb::Blob* data, + bool exclusive) { ntsa::Error error; @@ -3346,7 +3322,14 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( return error; } - error = d_encryption_sp->pushIncomingCipherText(*d_receiveBlob_sp); + // Pushing incoming cipher text may invoke the handshake callback, which + // may reset 'd_encryption_sp' if the handshake fails. Retain a + // reference to the encryption session so it is not destroyed while + // one of its member functions is still executing. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->pushIncomingCipherText(*d_receiveBlob_sp); if (NTCCFG_UNLIKELY(error)) { return error; } @@ -3442,8 +3425,98 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_receiveQueue.pushEntry(entry); } - if (NTCCFG_UNLIKELY(downgradeAbortively)) { + // Receiving the peer's TLS shutdown, or unencrypted leftovers after + // the encrypted data, downgrades the connection, which resets the + // encryption session: a cross-cutting transition that requires both + // mutexes. The lock order permits acquiring 'd_sendMutex' while + // holding 'd_receiveMutex'. + + if (NTCCFG_UNLIKELY(downgradeAbortively || + d_encryption_sp->isShutdownReceived())) + { + if (exclusive) { + return this->privateReceiveDowngrade(self, + downgradeAbortively); + } + + LockGuard sendLock(&d_sendMutex); + return this->privateReceiveDowngrade(self, downgradeAbortively); + } + + // Enqueue any outgoing cipher text generated while decrypting, e.g., + // the response to a TLS 1.3 key update. + + if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + if (exclusive) { + return this->privateSendOutgoingCipherText(self); + } + + LockGuard sendLock(&d_sendMutex); + return this->privateSendOutgoingCipherText(self); + } + + return ntsa::Error(); + } +} + +ntsa::Error StreamSocket::privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive) +{ + ntsa::Error error; + + if (!d_encryption_sp) { + return ntsa::Error(); + } + + if (NTCCFG_UNLIKELY(abortive)) { + if (!d_encryption_sp->isShutdownSent()) { + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_INITIATED); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeInitiated( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_reactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + + d_encryption_sp.reset(); + + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_COMPLETE); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeComplete(d_session_sp, + self, + event, + d_sessionStrand_sp, + d_reactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + else { + if (d_encryption_sp->isShutdownReceived()) { if (!d_encryption_sp->isShutdownSent()) { + error = d_encryption_sp->shutdown(); + if (error) { + return error; + } + if (d_session_sp) { ntca::DowngradeContext context; @@ -3459,10 +3532,17 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } } + } + + error = this->privateSendOutgoingCipherText(self); + if (error) { + return error; + } + if (d_encryption_sp->isShutdownFinished()) { d_encryption_sp.reset(); if (d_session_sp) { @@ -3472,98 +3552,52 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( event.setType(ntca::DowngradeEventType::e_COMPLETE); event.setContext(context); - ntcs::Dispatch::announceDowngradeComplete(d_session_sp, - self, - event, - d_sessionStrand_sp, - d_reactorStrand_sp, - self, - true, - &d_mutex); + ntcs::Dispatch::announceDowngradeComplete( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_reactorStrand_sp, + self, + true, + &d_receiveMutex); } } - else { - if (d_encryption_sp->isShutdownReceived()) { - if (!d_encryption_sp->isShutdownSent()) { - error = d_encryption_sp->shutdown(); - if (error) { - return error; - } - - if (d_session_sp) { - ntca::DowngradeContext context; - - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_INITIATED); - event.setContext(context); - - ntcs::Dispatch::announceDowngradeInitiated( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_reactorStrand_sp, - self, - true, - &d_mutex); - } - } - } - - if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { - bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); + } - while ( - NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) - { - error = - d_encryption_sp->popOutgoingCipherText(&cipherData); - if (error) { - return error; - } - } + return ntsa::Error(); +} - if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { - ntcq::SendState state; - state.setCounter(d_sendCounter++); - - error = this->privateSendRaw(self, - cipherData, - state, - ntca::SendOptions(), - ntca::SendContext(), - d_sendComplete); - if (error) { - return error; - } - } - } +ntsa::Error StreamSocket::privateSendOutgoingCipherText( + const bsl::shared_ptr& self) +{ + ntsa::Error error; - if (d_encryption_sp->isShutdownFinished()) { - d_encryption_sp.reset(); + bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); - if (d_session_sp) { - ntca::DowngradeContext context; + while (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + error = d_encryption_sp->popOutgoingCipherText(&cipherData); + if (error) { + return error; + } + } - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_COMPLETE); - event.setContext(context); + if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { + ntcq::SendState state; + state.setCounter(d_sendCounter++); - ntcs::Dispatch::announceDowngradeComplete( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_reactorStrand_sp, - self, - true, - &d_mutex); - } - } + error = this->privateSendRaw(self, + cipherData, + state, + ntca::SendOptions(), + ntca::SendContext(), + d_sendComplete); + if (error) { + return error; } - - return ntsa::Error(); } + + return ntsa::Error(); } ntsa::Error StreamSocket::privateDequeueReceiveBufferRaw( @@ -4012,16 +4046,10 @@ ntsa::Error StreamSocket::privateSendEncrypted( } if (cipherData.length() > 0) { - // The encrypted send path holds both 'd_mutex' and 'd_sendMutex'; - // suppress synchronous (recursive) completion so the raw send does not - // dispatch a callback while holding more than one mutex. - ntca::SendOptions rawOptions(options); - rawOptions.setRecurse(false); - error = this->privateSendRaw(self, cipherData, state, - rawOptions, + options, setting, callback); if (error) { @@ -4033,13 +4061,13 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so - // the completion callback is always deferred. + const bool defer = !options.recurse(); + callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - true, + defer, &d_sendMutex); } @@ -4071,16 +4099,10 @@ ntsa::Error StreamSocket::privateSendEncrypted( } if (cipherData.length() > 0) { - // The encrypted send path holds both 'd_mutex' and 'd_sendMutex'; - // suppress synchronous (recursive) completion so the raw send does not - // dispatch a callback while holding more than one mutex. - ntca::SendOptions rawOptions(options); - rawOptions.setRecurse(false); - error = this->privateSendRaw(self, cipherData, state, - rawOptions, + options, setting, callback); if (error) { @@ -4092,13 +4114,13 @@ ntsa::Error StreamSocket::privateSendEncrypted( sendEvent.setType(ntca::SendEventType::e_COMPLETE); sendEvent.setContext(setting); - // The encrypted send path holds both 'd_mutex' and 'd_sendMutex', so - // the completion callback is always deferred. + const bool defer = !options.recurse(); + callback.dispatch(self, sendEvent, ntci::Strand::unknown(), self, - true, + defer, &d_sendMutex); } @@ -4419,7 +4441,7 @@ ntsa::Error StreamSocket::privateOpen( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -4436,7 +4458,7 @@ void StreamSocket::processSourceEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; ntca::BindContext bindContext; @@ -4493,7 +4515,7 @@ void StreamSocket::processSourceEndpointResolution( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -4509,9 +4531,8 @@ void StreamSocket::processRemoteEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -4613,7 +4634,7 @@ void StreamSocket::processRemoteEndpointResolution( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) @@ -4677,12 +4698,23 @@ ntsa::Error StreamSocket::privateUpgrade( bdlf::MemFnUtil::memFn(&StreamSocket::privateEncryptionHandshake, this); - error = - d_encryption_sp->initiateHandshake(upgradeOptions, handshakeCallback); + // Initiating the handshake and pushing incoming cipher text may invoke + // the handshake callback, which resets 'd_encryption_sp' if the handshake + // fails. Retain a reference to the encryption session so it is not + // destroyed while one of its member functions is still executing, and + // stop if the failure of the upgrade has already been processed. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->initiateHandshake(upgradeOptions, handshakeCallback); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + // Push any unconsumed receive queue data that must be now considered // cipher text into the encryption session. @@ -4690,12 +4722,15 @@ ntsa::Error StreamSocket::privateUpgrade( // Push the receive queue data into the encryption session as incoming // cipher text. - error = - d_encryption_sp->pushIncomingCipherText(*d_receiveQueue.data()); + error = encryption->pushIncomingCipherText(*d_receiveQueue.data()); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + bdlbb::BlobUtil::erase(d_receiveQueue.data().get(), 0, d_receiveQueue.data()->length()); @@ -4813,7 +4848,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -4858,7 +4893,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -4918,7 +4953,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } d_encryption_sp.reset(); @@ -4937,7 +4972,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -5150,7 +5185,7 @@ void StreamSocket::privateRetryConnectToEndpoint( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } if (d_detachState.mode() == ntcs::DetachMode::e_INITIATED) { @@ -5537,7 +5572,6 @@ StreamSocket::StreamSocket( const bsl::shared_ptr& metrics, bslma::Allocator* basicAllocator) : d_object("ntcr::StreamSocket") -, d_mutex() , d_sendMutex() , d_receiveMutex() , d_transport(ntsa::Transport::e_UNDEFINED) @@ -5727,7 +5761,7 @@ ntsa::Error StreamSocket::open() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self); } @@ -5736,7 +5770,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport); } @@ -5746,7 +5780,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle); } @@ -5757,7 +5791,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket); } @@ -5769,7 +5803,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle, acceptor); } @@ -5781,7 +5815,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket, acceptor); } @@ -5801,7 +5835,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -5841,7 +5875,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -5870,7 +5904,7 @@ ntsa::Error StreamSocket::bind(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -5917,7 +5951,7 @@ ntsa::Error StreamSocket::connect(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -6068,7 +6102,7 @@ ntsa::Error StreamSocket::connect(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -6193,9 +6227,8 @@ ntsa::Error StreamSocket::upgrade( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6353,29 +6386,17 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, { bsl::shared_ptr self = this->getSelf(this); - // Fast path: an unencrypted send only needs the send mutex. The encryption - // object is co-guarded and may be examined under the send mutex; if it is - // present, escalate to also holding the state mutex (in canonical order), - // since the encrypted send path operates on the shared encryption object. + // Both unencrypted and encrypted sends need only the send mutex. The + // encryption object is co-guarded, so it is stable while the send mutex is + // held, and it is itself thread safe. Every path that pops outgoing + // cipher text and enqueues it to the send queue holds the send mutex, so + // TLS records are enqueued in the order they are generated. Pushing + // outgoing plain text never invokes the handshake callback (see + // 'ntci::Encryption'), so the encrypted send path never performs a + // cross-cutting transition. - { - LockGuard sendLock(&d_sendMutex); - if (NTCCFG_LIKELY(!d_encryption_sp)) { - return this->privateSend(self, data, options, callback); - } - } - - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - return this->privateSend(self, data, options, callback); -} -ntsa::Error StreamSocket::privateSend( - const bsl::shared_ptr& self, - const bdlbb::Blob& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback) -{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -6501,27 +6522,11 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, { bsl::shared_ptr self = this->getSelf(this); - // Fast path: an unencrypted send only needs the send mutex; escalate to - // also hold the state mutex (in canonical order) only when encrypted. - - { - LockGuard sendLock(&d_sendMutex); - if (NTCCFG_LIKELY(!d_encryption_sp)) { - return this->privateSend(self, data, options, callback); - } - } + // Both unencrypted and encrypted sends need only the send mutex. See the + // 'bdlbb::Blob' overload of this function. - LockGuard stateLock(&d_mutex); LockGuard sendLock(&d_sendMutex); - return this->privateSend(self, data, options, callback); -} -ntsa::Error StreamSocket::privateSend( - const bsl::shared_ptr& self, - const ntsa::Data& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback) -{ NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -6949,14 +6954,14 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntsa::Error StreamSocket::registerResolver( const bsl::shared_ptr& resolver) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver = resolver; return ntsa::Error(); } ntsa::Error StreamSocket::deregisterResolver() { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver.reset(); return ntsa::Error(); } @@ -6966,9 +6971,8 @@ ntsa::Error StreamSocket::registerManager( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (manager) { d_manager_sp = manager; @@ -6988,9 +6992,8 @@ ntsa::Error StreamSocket::registerManager( ntsa::Error StreamSocket::deregisterManager() { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_manager_sp.reset(); d_managerStrand_sp.reset(); @@ -7003,9 +7006,8 @@ ntsa::Error StreamSocket::registerSession( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (session) { d_session_sp = session; @@ -7035,9 +7037,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -7074,9 +7075,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -7106,9 +7106,8 @@ ntsa::Error StreamSocket::registerSessionCallback( ntsa::Error StreamSocket::deregisterSession() { - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_session_sp.reset(); d_sessionStrand_sp.reset(); @@ -7204,7 +7203,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -7243,7 +7242,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -7284,7 +7283,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -7306,7 +7305,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -7387,7 +7386,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -7476,9 +7475,8 @@ ntsa::Error StreamSocket::relaxFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7495,9 +7493,8 @@ ntsa::Error StreamSocket::applyFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7539,9 +7536,8 @@ ntsa::Error StreamSocket::cancel(const ntca::ConnectToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7567,9 +7563,8 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7603,7 +7598,7 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } this->privateFail(self, ntsa::Error(ntsa::Error::e_CANCELLED)); @@ -7652,7 +7647,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -7693,7 +7688,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -7705,9 +7700,8 @@ ntsa::Error StreamSocket::downgrade() { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); return this->privateDowngrade(self, ntca::DowngradeOptions()); } @@ -7716,9 +7710,8 @@ ntsa::Error StreamSocket::downgrade(const ntca::DowngradeOptions& options) { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (options.abortive().isNull() || !options.abortive().value()) { return this->privateDowngrade(self, options); @@ -7733,9 +7726,8 @@ ntsa::Error StreamSocket::shutdown(ntsa::ShutdownType::Value direction, { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7784,9 +7776,8 @@ ntsa::Error StreamSocket::release(ntsa::Handle* result, { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); *result = ntsa::k_INVALID_HANDLE; @@ -7822,9 +7813,8 @@ void StreamSocket::close(const ntci::CloseCallback& callback) { bsl::shared_ptr self = this->getSelf(this); - LockGuard stateLock(&d_mutex); - LockGuard sendLock(&d_sendMutex); LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); this->privateClose(self, callback); } @@ -7960,7 +7950,7 @@ ntsa::Endpoint StreamSocket::remoteEndpoint() const bsl::shared_ptr StreamSocket::sourceCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -7973,7 +7963,7 @@ bsl::shared_ptr StreamSocket::sourceCertificate() bsl::shared_ptr StreamSocket::remoteCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -7985,7 +7975,7 @@ bsl::shared_ptr StreamSocket::remoteCertificate() bsl::shared_ptr StreamSocket::privateKey() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { diff --git a/groups/ntc/ntcr/ntcr_streamsocket.h b/groups/ntc/ntcr/ntcr_streamsocket.h index 74b40d45..cecb86f0 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.h +++ b/groups/ntc/ntcr/ntcr_streamsocket.h @@ -93,34 +93,51 @@ class StreamSocket : public ntci::StreamSocket, ntccfg::Object d_object; // LOCKING STRATEGY // - // 'd_mutex' (the "state" mutex) guards the shared lifecycle and control - // state of the socket: the connect/upgrade machinery, the open, shutdown, - // and detach state machines' *transitions*, and the encryption object. - // - // 'd_sendMutex' guards the write path (the send queue, zero-copy queue, - // outgoing timestamping, and send-direction reactor interest). // 'd_receiveMutex' guards the read path (the receive queue and - // receive-direction reactor interest). These two leaf mutexes are - // independent so that sending is not serialized against receiving. + // receive-direction reactor interest) and the shared lifecycle and + // control state of the socket: the connect/upgrade machinery and the + // open, shutdown, and detach state machines' *transitions*. Conceptually, + // it guards the connectedness and potential readability of the socket. + // 'd_sendMutex' guards the separate write path (the send queue, zero-copy + // queue, outgoing timestamping, and send-direction reactor interest), so + // that sending is not serialized against receiving. + // + // The encryption object is itself thread safe. Encrypting outgoing data + // requires only 'd_sendMutex': every path that pops outgoing cipher text + // and enqueues it to the send queue holds 'd_sendMutex', so TLS records + // are enqueued in the order they are generated, and pushing outgoing plain + // text never invokes the handshake callback. + // + // Decrypting incoming data in the steady state (i.e., once the handshake + // has completed) requires only 'd_receiveMutex': the handshake callback + // can no longer be invoked. Decrypting incoming data while the handshake + // is in progress holds both mutexes, since it may complete or fail the + // handshake (a cross-cutting transition). // // 'd_flowControlState' is itself thread safe (lock-free), so a single // instance is shared by both directions: the send path applies/relaxes it // with 'e_SEND' under 'd_sendMutex' and the receive path with 'e_RECEIVE' // under 'd_receiveMutex'. // - // When more than one is held they are always acquired in the order - // 'd_mutex' -> 'd_sendMutex' -> 'd_receiveMutex'. A steady-state I/O drain - // holds only its own leaf; when it must perform a cross-cutting lifecycle - // transition (connection establishment, failure, shutdown) it first - // releases its leaf, then re-acquires in canonical order. + // When both are held they are always acquired in the order + // 'd_receiveMutex' -> 'd_sendMutex'. This order follows the flow of data: + // the read path may feed the write path, so it may acquire 'd_sendMutex' + // while holding 'd_receiveMutex', but the write path never acquires + // 'd_receiveMutex' while holding 'd_sendMutex'. Consequently, the read + // path performs every cross-cutting transition it discovers (enqueueing + // outgoing cipher text generated while decrypting, receiving the peer's + // TLS shutdown, end-of-file, or failure) in place, by additionally + // acquiring 'd_sendMutex' without releasing 'd_receiveMutex'. The write + // path holds only 'd_sendMutex'; when it must perform a cross-cutting + // lifecycle transition (connection establishment, failure, shutdown) it + // first releases 'd_sendMutex', then acquires both in canonical order. // // The observable lifecycle predicates ('d_openState', 'd_shutdownState', // 'd_detachState.mode()') are atomic and may be read without any lock. The // co-guarded members ('d_socket_sp', 'd_encryption_sp', 'd_session_sp', // 'd_sessionStrand_sp', 'd_manager_sp', 'd_managerStrand_sp') are written - // only while holding all three mutexes and may therefore be read while - // holding any one of them. - mutable Mutex d_mutex; + // only while holding both mutexes and may therefore be read while holding + // either one of them. mutable Mutex d_sendMutex; mutable Mutex d_receiveMutex; ntsa::Transport::Value d_transport; @@ -272,13 +289,13 @@ class StreamSocket : public ntci::StreamSocket, const bsl::string& details); /// Process the readability of the socket by performing one read - /// iteration. If the specified 'defer' flag is true, force all event - /// announcements and callback invocations made during the iteration to be - /// deferred (used when the iteration runs while holding more than one - /// mutex, e.g. the encrypted read path). + /// iteration. If the specified 'exclusive' flag is true, the caller holds + /// both 'd_receiveMutex' and 'd_sendMutex', and all event announcements + /// and callback invocations made during the iteration are deferred. Otherwise, the caller holds only 'd_receiveMutex'. Return + /// the error. ntsa::Error privateSocketReadableIteration( const bsl::shared_ptr& self, - bool defer); + bool exclusive); /// Process the writability of the socket indicating the connection /// is established. @@ -387,7 +404,8 @@ class StreamSocket : public ntci::StreamSocket, bool lock); /// Enable copying from the socket buffers in the specified 'direction'. - /// The behavior is undefined unless 'd_mutex' is locked. + /// The behavior is undefined unless the mutex guarding the 'direction' + /// (or both mutexes, for 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateRelaxFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, @@ -396,7 +414,8 @@ class StreamSocket : public ntci::StreamSocket, /// Disable copying from socket buffers in the specified 'direction' /// according to the specified 'mode'. The behavior is undefined unless - /// 'd_mutex' is locked. + /// the mutex guarding the 'direction' (or both mutexes, for + /// 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateApplyFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, @@ -447,11 +466,33 @@ class StreamSocket : public ntci::StreamSocket, ntsa::Handle foreignHandle); /// Dequeue data from the socket receive buffer. Append to the - /// specified 'data' the data dequeued. Return the error. + /// specified 'data' the data dequeued. If the specified 'exclusive' flag + /// is true, the caller holds both 'd_receiveMutex' and 'd_sendMutex'. + /// Otherwise, the caller holds only 'd_receiveMutex', 'd_sendMutex' is + /// additionally acquired as necessary, and the behavior is undefined + /// unless the encryption handshake, if any, has completed. Return the + /// error. ntsa::Error privateDequeueReceiveBuffer( const bsl::shared_ptr& self, ntsa::ReceiveContext* context, - bdlbb::Blob* data); + bdlbb::Blob* data, + bool exclusive); + + /// Complete the downgrade from encrypted to unencrypted communication + /// initiated by the receipt of the peer's TLS shutdown, or, if the + /// specified 'abortive' flag is true, by the receipt of unencrypted + /// leftovers after the encrypted data. Enqueue any outgoing cipher text + /// generated as a result. The behavior is undefined unless both + /// 'd_receiveMutex' and 'd_sendMutex' are locked. Return the error. + ntsa::Error privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive); + + /// Pop all outgoing cipher text from the encryption session and enqueue + /// it for transmission. The behavior is undefined unless 'd_sendMutex' is + /// locked. Return the error. + ntsa::Error privateSendOutgoingCipherText( + const bsl::shared_ptr& self); /// Dequeue raw or encrypted data from the socket receive buffer. Append /// to the specified 'data' the data dequeued . Return the error. @@ -460,24 +501,6 @@ class StreamSocket : public ntci::StreamSocket, ntsa::ReceiveContext* context, bdlbb::Blob* data); - /// Enqueue the specified 'data' for transmission according to the - /// specified 'options', invoking the specified 'callback' on completion. - /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the - /// socket is encrypted, 'd_mutex' as well). Return the error. - ntsa::Error privateSend(const bsl::shared_ptr& self, - const bdlbb::Blob& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback); - - /// Enqueue the specified 'data' for transmission according to the - /// specified 'options', invoking the specified 'callback' on completion. - /// The behavior is undefined unless 'd_sendMutex' is locked (and, if the - /// socket is encrypted, 'd_mutex' as well). Return the error. - ntsa::Error privateSend(const bsl::shared_ptr& self, - const ntsa::Data& data, - const ntca::SendOptions& options, - const ntci::SendCallback& callback); - /// Rearm the interest in the writability of the socket in the reactor, /// if necessary. void privateRearmAfterSend(const bsl::shared_ptr& self); diff --git a/groups/ntc/ntctls/ntctls_plugin.cpp b/groups/ntc/ntctls/ntctls_plugin.cpp index 047eb1fc..4d6cae3d 100644 --- a/groups/ntc/ntctls/ntctls_plugin.cpp +++ b/groups/ntc/ntctls/ntctls_plugin.cpp @@ -9451,6 +9451,15 @@ ntsa::Error Session::process(LockGuard* lock) return ntsa::Error(ntsa::Error::e_INVALID); } + // Clear any stale errors in this thread's OpenSSL error queue, so that + // 'SSL_get_error' reports the outcome of the operations below rather than + // an unrelated, previously-unconsumed error. Otherwise, a stale error could + // be misinterpreted as a handshake failure, and the handshake callback + // could be invoked from a call other than 'initiateHandshake' or + // 'pushIncomingCipherText'. + + ERR_clear_error(); + bool announceHandshakeComplete = false; if (SSL_in_init(d_ssl_p)) { @@ -9476,12 +9485,17 @@ ntsa::Error Session::process(LockGuard* lock) d_handshakeCallback = ntci::Encryption::HandshakeCallback(); - lock->release()->unlock(); + // The handshake callback is empty if the failure of the handshake + // has already been announced. - handshakeCallback( - ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), - bsl::shared_ptr(), - description); + if (handshakeCallback) { + lock->release()->unlock(); + + handshakeCallback( + ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), + bsl::shared_ptr(), + description); + } return ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED); } @@ -10030,6 +10044,8 @@ ntsa::Error Session::initiateHandshake( d_handshakeCallback = callback; + ERR_clear_error(); + rc = SSL_do_handshake(d_ssl_p); if (rc != 1) { @@ -10267,6 +10283,8 @@ ntsa::Error Session::shutdown() enum { k_SHUTDOWN_STARTING = 0, k_SHUTDOWN_COMPLETE = 1 }; + ERR_clear_error(); + int rc = SSL_shutdown(d_ssl_p); if (rc != k_SHUTDOWN_STARTING && rc != k_SHUTDOWN_COMPLETE) {