Add a connection pool #105
Conversation
Thank you! I'll add comments to explain everything. @Lukasa @t089 I will fix the parts where you noticed issues, thank you for your feedback! There is also quite a lot of code that needs to be cleaned. The main goal of doing this draft PR right now is to be ensure breaking changes can be added before the I think #102 already adresses what's needed for thread safety issues and or the pool to have enough informations to make the better decisions for performance, but there is another part to consider: retried requests. About this I have two ideas:
I'm not really sure of what the best thing to do would be so I would appreciate to have your opinion about this, if needed I can also open it as a separate issue. |
I'm quite tempted by this approach, as it allows us to potentially shim a solution in by having an automatic-retry delegate that can be composed with a user-provided delegate in some way. In general, trying to do too much is likely to go badly for us. I think it may be a good idea to explicitly say that retries must be handled by delegates (and to add some API surface for doing so), as it allows us to punt on making the decision any further. However, @weissi may be opposed to adding more API for this. |
Just to be sure of what you mean exactly by "handled by delegates": do you mean something similar to |
|
The 0d33412 commit adds new options (passed as |
I think I'm down with that. I think we should make automatic retrying very easy for somebody to implement in their own delegate. That could work either with composition as @Lukasa proposed or possibly with a default implementation? |
|
Im trying to bring this back to light because it speeds up AsyncHTTPRequest considerably. Without With Connection pooling And then I also ran it on an iOS device, 10 requests, this is a total combined time in release mode. I worked on the PR a bit, but cant seem to get the decompression test to pass. |
PopFlamingo
changed the title
|
The PR is now ready for review! 🙂 cc @weissi @Lukasa @t089 @tomerd @artemredkin Thanks to the precious help of @kylebrowning this PR now includes the latest changes of master and all tests are passing, the benchmarks are really insightful too and let us ensure that these changes are beneficial to the performance. What do you think about the connection pool implementation? What can be improved / made clearer? Thank you! A few things to check in all cases before merging:
|
| /// throw the appropriate error if needed. For instance, if its internal connection pool has any non-released connections, | ||
| /// this indicate shutdown was called too early before tasks were completed or explicitly canceled. | ||
| /// In general, setting this parameter to `true` should make it easier and faster to catch related programming errors. | ||
| public func syncShutdown(requiresCleanClose: Bool) throws { |
There was a problem hiding this comment.
do we need this as public API? I think this can remain internal, no?
There was a problem hiding this comment.
@weissi I think it might need to be public because syncShutdown() calls syncShutdown(requiresCleanClose: false) for backwards compatibility, but since users should preferably do a clean close, they need to be able to call syncShutdown(requiresCleanClose: true). WDYT?
There was a problem hiding this comment.
but if you were a user of this library, wouldn't you be super confused on which shutdown to use and what parameter to pass? I'd just not expose syncShutdown(requiresCleanClose:) (can be used in tests of course). If we feel that it's super important, we can always make it public later
There was a problem hiding this comment.
I was worried about the user shutting down the client too early and having difficulties understanding why all their tasks are getting cancelled, but I recognise requiresCleanClose is a bit confusing and the docs I added are not as much clear as they could be. I'll remove it for now then, never too late to add it later indeed.
There was a problem hiding this comment.
Were you planning to remove this?
| // if the `throw` wasn't delayed until everything | ||
| // is executed | ||
| if closeError == nil { | ||
| closeError = error |
There was a problem hiding this comment.
Is there any other possible error than ChannelError.alreadyClosed on close? If there is a reproducible one it might be good to add a test that makes it so that this function throws
There was a problem hiding this comment.
depends on the channel handlers in the pipeline to be precise. I think ignoring alreadyClosed and surfacing everything else makes sense
|
@artemredkin why not start with fixed and add config later in a separate PR? |
|
@artemredkin Thanks! Latest commit fixes the bugs found by @weissi |
| } | ||
|
|
||
| /// Get a new connection from this provider | ||
| func getConnection(preference: HTTPClient.EventLoopPreference) -> EventLoopFuture<Connection> { |
There was a problem hiding this comment.
In some sense it doesn't matter which event loop this future returns on, so long as it is always consistent about that choice. Otherwise it becomes impossible to write thread-safe programs. Given that we allow users to express an event loop preference here it seems sensible to follow the idea that the event loop should be returned on their preference.
There are two good choices of which event loop the future callback should run on:
- The event loop of the returned connection.
- The event loop of the caller.
Both of these are defensible, depending on what the user is exactly trying to do. It doesn't matter too much which of those we choose, but we should be attempting to guarantee that this is the case. Here we lose track of what the event loop of the future we're returning is, so it'd be good to keep track of it.
| /// throw the appropriate error if needed. For instance, if its internal connection pool has any non-released connections, | ||
| /// this indicate shutdown was called too early before tasks were completed or explicitly canceled. | ||
| /// In general, setting this parameter to `true` should make it easier and faster to catch related programming errors. | ||
| public func syncShutdown(requiresCleanClose: Bool) throws { |
There was a problem hiding this comment.
Were you planning to remove this?
|
@Lukasa Thanks for the NITs, I'll check and fix your other reviews soon |
|
Great, please @ me when you're ready for a re-review. |
|
This PR is ready for a new round of reviews! 🙂 If you see no additional issues, I think the PR is ready to be merged. I'll squash the commits and make a correct commit message when the PR is approved. Thanks |
motivation: Better performance thanks to connection reuse changes: - Added a connection pool for HTTP/1.1 - All requests automatically use the connection pool - Up to 8 parallel connections per (scheme, host, port) - Multiple additional unit tests
|
YAY! |
|
Thank you all for your reviews and advices, I learned a lot doing this! 🙂 Special thanks to you too @kylebrowning for bringing this back up a few months ago! |
This commit adds a basic connection pool implementation. It is still extremely WIP but includes breaking changes that must probably be taken care of before the
1.0.0tag (see the comment below).I'll add as many comments as possible tonight to explain parts that need to.
Let me know what you think! 🙂
There are many things to fix before the PR is truly ready for review/merge:
Making
HTTPBinready for pooling tests:Misc issues/improvements
ConnectionProviderwill not add the proper SSL handler forhttpsconnectionsLocks for as long as its currently the caseTesting
Is this needed?
Not in this commit:
HTTP/2:
HTTPBin