Repository navigation
Fix data races in NetworkTransport and MockNetworkConnection - #301
Conversation
NetworkTransport.attemptConnection() was writing connection.stateUpdateHandler from two contexts: the actor's executor (setting the handler) and AsyncStream's onTermination callback (clearing it to nil), which can fire on any thread. Fix: remove onTermination and use defer to clear the handler on the actor's executor after the for-await loop exits, regardless of normal return or throw. MockNetworkConnection was @unchecked Sendable with entirely unprotected mutable state. The Task { @mainactor in } wrappers were inconsistent: simulateFailure wrote mockError directly while receive read it on @mainactor, and stateUpdateHandler was set from NetworkTransport's actor executor while being read/written on @mainactor. Fix: add NSLock protecting all mutable state; snapshot handler before calling it to avoid holding the lock during the callback.
03776c3 to
bef2f07
Compare
| let (state, error, data): (NWConnection.State, Swift.Error?, Data?) = lock.withLock { | ||
| let d = dataToReceive.isEmpty ? nil : dataToReceive.removeFirst() | ||
| return (mockState, mockError, d) | ||
| } | ||
|
|
||
| let data = self.dataToReceive.removeFirst() | ||
| completion(data, nil, self.dataToReceive.isEmpty, nil) | ||
| if state == .cancelled { | ||
| completion(nil, nil, true, NWError.posix(POSIXErrorCode.ECANCELED)) | ||
| return | ||
| } | ||
| if let error { | ||
| completion(nil, nil, false, error as? NWError) | ||
| return | ||
| } | ||
| let isLast = lock.withLock { dataToReceive.isEmpty } | ||
| completion(data ?? Data(), nil, data == nil ? false : isLast, nil) |
There was a problem hiding this comment.
@varun06 This removes a queued item before the cancelled/error checks, so it gets dropped on those paths. isLast is also read under a separate lock. Do both under one lock:
| let (state, error, data): (NWConnection.State, Swift.Error?, Data?) = lock.withLock { | |
| let d = dataToReceive.isEmpty ? nil : dataToReceive.removeFirst() | |
| return (mockState, mockError, d) | |
| } | |
| let data = self.dataToReceive.removeFirst() | |
| completion(data, nil, self.dataToReceive.isEmpty, nil) | |
| if state == .cancelled { | |
| completion(nil, nil, true, NWError.posix(POSIXErrorCode.ECANCELED)) | |
| return | |
| } | |
| if let error { | |
| completion(nil, nil, false, error as? NWError) | |
| return | |
| } | |
| let isLast = lock.withLock { dataToReceive.isEmpty } | |
| completion(data ?? Data(), nil, data == nil ? false : isLast, nil) | |
| let (state, error, data, isLast): (NWConnection.State, Swift.Error?, Data?, Bool) = | |
| lock.withLock { | |
| guard mockState != .cancelled, mockError == nil, !dataToReceive.isEmpty else { | |
| return (mockState, mockError, nil, false) | |
| } | |
| let data = dataToReceive.removeFirst() | |
| return (mockState, mockError, data, dataToReceive.isEmpty) | |
| } | |
| if state == .cancelled { | |
| completion(nil, nil, true, NWError.posix(POSIXErrorCode.ECANCELED)) | |
| return | |
| } | |
| if let error { | |
| completion(nil, nil, false, error as? NWError) | |
| return | |
| } | |
| completion(data ?? Data(), nil, isLast, nil) |
There was a problem hiding this comment.
Good catch on both — applied your suggestion in 419a7b6. State checks, the dequeue, and isLast now happen under a single lock, so a queued item is no longer dropped on the cancelled/error paths. Full test suite passes locally. Thanks!
MockNetworkConnection.receive removed a queued item before checking for a cancelled or failed connection, dropping that item on those paths, and read isLast under a separate lock acquisition. Check state, dequeue, and compute isLast together under a single lock. Addresses review feedback on modelcontextprotocol#301.
akhilcb
left a comment
There was a problem hiding this comment.
Thanks @varun06. 419a7b6 fixes the mock receive path. The cancelled and error checks, the dequeue and the isLast read now all happen under one lock, so a queued item can't be dropped on the cancelled or error paths anymore. CI is green across macOS, Linux and conformance. LGTM.
Motivation and Context
NetworkTransport.attemptConnection()setconnection.stateUpdateHandleron theactor's executor but cleared it from
AsyncStream'sonTermination, which can run onany thread. The handler is now cleared with
deferon the actor after thefor awaitloop exits.
MockNetworkConnectionwas@unchecked Sendablewith unprotectedmutable state accessed from both the transport actor and
@MainActortasks. Its stateis now lock-protected, and handlers are snapshotted before being invoked.
Related upstream work in the same area: #283, #216, #213.
How Has This Been Tested?
swift testpasses. (A TSan run ofNetworkTransportTestscurrently aborts onmaininHeartbeatparsing — a misalignedload(as: UInt64.self)— which is unrelated to this change.)Breaking Changes
None.
Types of changes