hyper

hyper copied to clipboard

In the previous implementation the connect_to future was spawned to let it finish when the checkout future won the race. However, in the event of the user dropping the ResponseFuture, the connect_to future was canceled as well, and all the http2 requests waiting for the connection would error as a result. This commit prevents that by spawning the connect_to future invariably (once it starts) thus covering both cases.

Fixes #3199

I've chosen a slightly different approach than that suggested in #3199, and also taken the liberty to remove common/lazy.rs, as it is no longer used with this approach. Let me know what you think, I'm open to trying other options.

taken the liberty to remove common/lazy.rs, as it is no longer used with this approach.

I think this still needs to be left in. It's there to prevent always starting a connect call, if a checkout would have been just fine. There used to be a unit test for that, but I don't think it survived the transition from futures 0.1 to async/await... (https://github.com/hyperium/hyper/commit/13741f5145eb3dc894d2bc8d8486fc51c29e2e41)

I thought the particularity of Lazy is to implement Started, which we use to check if the connect future has already started when checkout wins. However, as in this PR we are spawning the connect future anyway, we no longer need to check if it has already started.

However if I'm not mistaken Lazy is not lazier than any other future, right? Note that the connector.connect() future is spawned only after the future returned by connect_to is polled for the first time. The following tests currently cover the requisite you mention (I know because my initial implementation didn't comply with it and these tests fail):

    dispatch_impl::alpn_h2
    dispatch_impl::client_keep_alive_0
    dispatch_impl::client_keep_alive_eager_when_chunked
    dispatch_impl::client_keep_alive_when_response_before_request_body_ends

@seanmonstar could you approve the workflow again please 🙏 ?

However, as in this PR we are spawning the connect future anyway

I'm thinking through this... Is this true always? I'm thinking that at least in the HTTP/1 case, after grabbing the checkout and connect_to futures and racing them, if the checkout wins immediately and the connect_to was never "started", there's no need to spawn it and trigger a new idle connection, right?

However, as in this PR we are spawning the connect future anyway

I'm thinking through this... Is this true always? I'm thinking that at least in the HTTP/1 case, after grabbing the checkout and connect_to futures and racing them, if the checkout wins immediately and the connect_to was never "started", there's no need to spawn it and trigger a new idle connection, right?

I misspoke in the previous comment. What you say is exactly how it behaves (also for http2), and it is precisely the reason I claim the Lazy struct is no longer needed. What I meant is that the current implementation tries to run the connect_to future in the same task, and in the event of an error (if the connect_to future has started) spawns it in a background task to let it finish. This PR essentially removes this attempt to avoid spawning a new task for the connection attempt, and does it invariably, but instead of spawning the connect_to future, it spawns the connector.connect() future within (thus not spawning anything if the connect_to future is never polled).

Let me know how do you want to resolve this. If I haven't convinced you about the Lazy I'll revert that change, no worries!

Sorry, I was wrapping up a few other things, and wanted to pay this more attention to be sure I understand, because I feel this is pretty subtle. I'll take another look!

One downside with this is it requires spawning another task. Would it be possible to do this without a new task? Or would it require way too much juggling of the pool to get it to work?

I'll give it a try!