Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(client): run connect_to to completion even if ResponseFuture is canceled #3205

Open
wants to merge 5 commits into
base: 0.14.x
Choose a base branch
from

Conversation

arnauorriols
Copy link

@arnauorriols arnauorriols commented Apr 18, 2023

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

…is canceled

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 hyperium#3199
@arnauorriols
Copy link
Author

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.

@seanmonstar
Copy link
Member

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... (13741f5)

@arnauorriols
Copy link
Author

arnauorriols commented Apr 19, 2023

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

@arnauorriols
Copy link
Author

@seanmonstar could you approve the workflow again please 🙏 ?

@seanmonstar
Copy link
Member

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?

@arnauorriols
Copy link
Author

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).

@arnauorriols
Copy link
Author

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!

@seanmonstar
Copy link
Member

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!

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see what you mean about the lazy. It's become a different form of lazy.

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?

return Either::Right(future::err(canceled));
}
};
Either::Left(Box::pin(async move {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see, by putting the call to connector.connect() inside this sub future, async move, it won't call connect() until the first poll. Got it.

tests/client.rs Outdated
assert!(
// The second response has not been canceled and it is still waiting for the connection
// that will never arrive
future::poll_fn(|ctx| Poll::Ready(Pin::new(&mut res2).poll(ctx).is_pending())).await
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But will it actually fulfill with the connection? It'd be good to test that. Otherwise, a lost future here would also make this test pass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've implemented the suggestion. Thanks!

@arnauorriols
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants