[HTTPConnectionPool] Implementation switch#427
Merged
Uh oh!
There was an error while loading. Please reload this page.
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
fabianfettforce-pushed the ff-switch-implementation branch from 1fe5485 to efaf97cCompare
fabianfett commented Sep 14, 2021
Comment on lines 715 to +724
| func succeed<Delegate:HTTPClientResponseDelegate>(promise:EventLoopPromise<Response>?, | ||
| with value:Response, | ||
| delegateType:Delegate.Type, | ||
| closing:Bool){ | ||
| self.releaseAssociatedConnection(delegateType: delegateType, | ||
| closing: closing).whenSuccess{ | ||
| promise?.succeed(value) | ||
| } | ||
| promise?.succeed(value) | ||
| } | ||
| func fail<Delegate:HTTPClientResponseDelegate>(with error:Error, | ||
| delegateType:Delegate.Type){ | ||
| iflet connection =self.connection { | ||
| self.releaseAssociatedConnection(delegateType: delegateType, closing:true) | ||
| .whenSuccess{ | ||
| self.promise.fail(error) | ||
| connection.channel.close(promise:nil) | ||
| } | ||
| }else{ | ||
| // this is used in tests where we don't want to bootstrap the whole connection pool | ||
| self.promise.fail(error) | ||
| } | ||
| } | ||
| func releaseAssociatedConnection<Delegate:HTTPClientResponseDelegate>(delegateType:Delegate.Type, | ||
| closing:Bool)->EventLoopFuture<Void>{ | ||
| iflet connection =self.connection { | ||
| // remove read timeout handler | ||
| return connection.removeHandler(IdleStateHandler.self).flatMap{ | ||
| connection.removeHandler(TaskHandler<Delegate>.self) | ||
| }.map{ | ||
| connection.release(closing: closing, logger:self.logger) | ||
| }.flatMapError{ error in | ||
| fatalError("Couldn't remove taskHandler: \(error)") | ||
| } | ||
| }else{ | ||
| // TODO: This seems only reached in some internal unit test | ||
| // Maybe there could be a better handling in the future to make | ||
| // it an error outside of testing contexts | ||
| returnself.eventLoop.makeSucceededFuture(()) | ||
| } | ||
| self.promise.fail(error) |
MemberAuthor
There was a problem hiding this comment.
Those functions are not hit from the new implementation. However they are hit from the old implementation. Since we don't want to remove the old implementation in this pr, they should stay for now. We will be able to remove them in a follow up pr.
fabianfett commented Sep 14, 2021
| lethttpClient=HTTPClient(eventLoopGroupProvider:.shared(self.clientGroup)) | ||
| defer{ | ||
| XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose:true)) | ||
| XCTAssertNoThrow(try httpClient.syncShutdown()) |
MemberAuthor
There was a problem hiding this comment.
In the new implementation we can not make guarantees about the order of succeeding/failing the request (which will lead to a call to syncShutdown) and marking the connection as idle. For this reason we can not enforce a clean shutdown here.
fabianfettforce-pushed the ff-switch-implementation branch 7 times, most recently from 5601745 to 127abc3Comparefabianfettforce-pushed the ff-switch-implementation branch 2 times, most recently from a45c6a7 to 7cd68cbComparefabianfett changed the title fabianfettforce-pushed the ff-switch-implementation branch 2 times, most recently from 7deafde to 64eb025Compare
weissi reviewed Sep 22, 2021
| ("testRequestFinishesAfterRedirectIfServerRespondsBeforeClientFinishes", testRequestFinishesAfterRedirectIfServerRespondsBeforeClientFinishes), | ||
| ("testProxyStreaming", testProxyStreaming), | ||
| ("testProxyStreamingFailure", testProxyStreamingFailure), | ||
| ("testUploadStreamingBackpressure", testUploadStreamingBackpressure), |
Contributor
There was a problem hiding this comment.
do we have replacements for these tests?
MemberAuthor
There was a problem hiding this comment.
Yes. This is now tested way lower in the stack on the connection directly.
Use RequestOptions
fabianfettforce-pushed the ff-switch-implementation branch from 64eb025 to a5df5dbCompare
Lukasa approved these changes Sep 24, 2021
Collaborator
Lukasa left a comment
Lukasa left a comment There was a problem hiding this comment.
Nice, great work @fabianfett, this looks really good! ✨
MemberAuthor
fabianfett commented Sep 24, 2021
Apparently we have a new flaky test (see swift-nightly). Will fix in a follow up PR. Merge this one. |
This was referenced Sep 24, 2021
fabianfett added a commit that referenced this pull request Sep 24, 2021
### Motivation In #427 we switched the implementation over to our new `HTTPConnectionPool`. When we did this, we removed the possibility to access the requests underlying channel. For this reason we needed to remove a number of tests from `HTTPClientInternalTests`. As @weissi pointed out, we should at least still integration test downstream backpressure. This pr replicates the behavior from `HTTPClientInternalTests.testUploadStreamingBackpressure` directly on the `HTTP1Connection`. ### Changes - Move and adjust test from `HTTPClientInternalTests.testUploadStreamingBackpressure` - Rename the test to `testDownloadStreamingBackpressure` since we only test incoming bytes backpressure. ### Result An integration backpressure test, that survived #427.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
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.
This is the final pr to switch the implementation over to our new HTTPConnectionPool and Connections. There are some outstanding changes (#425, #426) that we want to land in smaller prs beforehand, but for everyone interested, this is your chance to play with the new implementation.
The failing unit tests are tests related logging, we will need to fix this before we can land this pr.