Fix crash when a connection release races with its own close/fail - #3
Merged
Conversation
http1ConnectionReleased force-unwrapped releaseConnection(_:), crashing
via preconditionFailure if the connectionID was no longer in the state
machine's connections array. Its sibling http1ConnectionClosed already
handled this gracefully (guard let ... else { return .none }), because
the same race exists for both events: the channel's closeFuture and
taskCompleted() callbacks run on the same EventLoop but their relative
order depends on I/O timing, so a connection can be torn down (idle
timeout, peer closing early, HTTP1->HTTP2 migration) right before its
queued release event is processed.
Found via the new Apple platform CI: enabling apple-tests exposed this
on iOS/iPadOS/watchOS simulators (crashing consistently around
testBiDirectionalStreamingEarly200 with "Fatal error: A connection
that we don't know was released? Something is very wrong..."), while
macOS/tvOS/visionOS/Catalyst didn't hit the race in the time it takes
to run the suite. Upstream's own CI never ran this suite against an
Apple platform at all, so the bug had no way of surfacing there.
Same fix applied at both call sites (HTTP1StateMachine and
HTTP2StateMachine), mirroring the existing http1ConnectionClosed
pattern in each.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found while validating the new
enable-apple-testsCI step on #2:iOS/iPadOS/watchOScrashed consistently aroundtestBiDirectionalStreamingEarly200with:macOS/tvOS/visionOS/Catalyst didn't hit it in the time it takes to run the suite. Upstream's own CI never ran this test suite against any Apple platform, so this had no way of surfacing before.Root cause:
http1ConnectionReleasedforce-unwrappedreleaseConnection(_:), crashing viapreconditionFailureif the connection was no longer tracked. Its siblinghttp1ConnectionClosedalready handles that same situation gracefully (guard let ... else { return .none }) — the two events (channel.closeFutureandtaskCompleted()) run on the same EventLoop, but their relative order depends on I/O timing, so a connection can be torn down (idle timeout, peer closing early, HTTP1→HTTP2 migration) right before its already-queued release event gets processed. This asymmetry existed at both call sites:HTTP1StateMachine.http1ConnectionReleasedandHTTP2StateMachine.http1ConnectionReleased.Fix:
HTTP1Connections.releaseConnection(_:)now returns(Int, IdleConnectionContext)?instead of crashing on an unknown connection, and both call sites handlenilthe same way theirhttp1ConnectionClosedsibling already does.Test plan
swift build— cleanswift test --filter HTTPConnectionPool— 100/100 passswift test --filter "HTTPClientTests|HTTPClientStructuredConcurrencyTests"locally — no crash; the only 2 failures (testConnectTimeout×2) reproduce identically onbetawithout this change (pre-existing local-environment flakiness, unrelated)betafirst to get realapple-tests(iOS/iPadOS/watchOS) coverage on this PR — happy to rebase/retarget once that lands, or this can be merged after Replace swift-nio CI pipeline with request-dl/.github's swift-ci.yaml #2 and re-verified onbetadirectly.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com