Tolerate new request after connection error happened #688
Conversation
This file contains 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
dnadoba
added
the
🔼 needs-minor-version-bump
dnadoba
commented
May 15, 2023
Comment on lines
-185
to
196
| case .initialized, .closing, .inRequest: | ||
| case .initialized, .inRequest: | ||
| // These states are unreachable as the connection pool state machine has put the | ||
| // connection into these states. In other words the connection pool state machine must | ||
| // be aware about these states before the connection itself. For this reason the | ||
| // connection pool state machine must not send a new request to the connection, if the | ||
| // connection is `.initialized`, `.closing` or `.inRequest` | ||
| preconditionFailure("Invalid state: \(self.state)") | ||
| fatalError("Invalid state: \(self.state)") | ||
|
|
||
| case .closed: | ||
| case .closing, .closed: | ||
| // The remote may have closed the connection and the connection pool state machine | ||
| // was not updated yet because of a race condition. New request vs. marking connection | ||
| // as closed. |
There was a problem hiding this comment.
This is the actual bug fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
A connection and the connection pool may be on different threads and therefore run concurrently. If a connection error happens the connections closes itself and notifies the pool about it. While this is happening the connection pool could already decide to run a new connection on the closing connection. We need to tolerate this happening. We already partially do but only if the connection is fully closed but not currently still closing.
Result
preconditionFailures inHTTP1ConnectionStateMachine.swifttofatalErrors to see the message in release builds.closingin addition to.closedinrunNewRequestResult
AHC no longer crashes in the race condition described above.