Fix HTTP1 to HTTP2 migration while shutdown is in progress #530
Conversation
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
### Motivation Calling `HTTPClient.shutdown()` may never return if connections are still starting and one new established connection results in a state migration (i.e. from HTTP1 to HTTP2 or vice versa). We forgot to migrate the shutdown state. This could result in a large dealy until `.shutdown()` returns because we wait until connections are closed because of idle timeout. Worse, it could also never return if more requests are queued because the connections would not be idle and therefore not close itself. ###Changes - Mirgrate shutdown state too - add tests for this specific case
dnadoba
force-pushed
the
dn-fix-migration-during-shutdown
branch
from
7604763 to
54a3a68
Compare
fabianfett
reviewed
Dec 17, 2021
| @@ -670,6 +673,89 @@ class HTTPConnectionPool_HTTP2StateMachineTests: XCTestCase { | |||
| XCTAssertTrue(queuer.isEmpty) | |||
| } | |||
|
|
|||
| func testMigrationFromHTTP1ToHTTP2WhileShuttingDown() { | |||
There was a problem hiding this comment.
I think this test case is a little to complex... let's just focus on the thing that we actually observed.
- run request -> opens new connection (expect h1)
- shutdown pool -> queued request shall be cancelled
- connection creation is done as h2 -> connection shall be closed immediately and shutdown is done.
then we should also have this vica versa.
- run request -> opens new connection
- connection creation is done as h2 -> schedule request on h2 connection
- connection is done with the single request
- idle timeout closes h2 connection
- new request comes in. we create a new connection (expect h2)
- shutdown pool -> cancel queued request
- connection creation is done as h1 -> connection shall be closed immediately and shutdown is done.
dnadoba
force-pushed
the
dn-fix-migration-during-shutdown
branch
from
6796ea0 to
8a7db19
Compare
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
Calling
HTTPClient.shutdown()may never return if connections are still starting and one new established connection results in a state migration (i.e. from HTTP1 to HTTP2 or vice versa). We forgot to migrate the shutdown state. This could result in a large delay until.shutdown()returns because we wait until connections are closed because of idle timeout. Worse, it could also never return if more requests are queued because the connections would not be idle and therefore not close itself.Changes