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

provide ability to request a fresh connection #2605

Open
rcoh opened this issue Jul 26, 2021 · 6 comments
Open

provide ability to request a fresh connection #2605

rcoh opened this issue Jul 26, 2021 · 6 comments
Labels
A-client Area: client. B-rfc Blocked: More comments would be useful in determine next steps. C-feature Category: feature. This is adding a new feature.

Comments

@rcoh
Copy link

rcoh commented Jul 26, 2021

Is your feature request related to a problem? Please describe.
Certain retry behaviors specify a fresh connection (and sometimes a full DNS re-lookup) to direct traffic away from what might be a problematic host. I need the ability to somehow mark a request to Hyper as not reusing a connection that may be in the pool

Describe the solution you'd like
I can imagine a few ways the API might be structured, but in general I'd like some way as marking a request as "needs fresh connection", potentially giving a reference to the old connection / response which was problematic.

Describe alternatives you've considered
I could rebuild the entire client & pool when a request fails in a way that necessitates a re-lookup. However, it seems like that may cause unnecessary churn on the client side.

Additional context
https://docs.aws.amazon.com/AmazonS3/latest/userguide/optimizing-performance-design-patterns.html#optimizing-performance-timeouts-retries

@rcoh rcoh added the C-feature Category: feature. This is adding a new feature. label Jul 26, 2021
@seanmonstar seanmonstar added A-client Area: client. B-rfc Blocked: More comments would be useful in determine next steps. labels Aug 26, 2021
@seanmonstar
Copy link
Member

I forgot to comment this, but one possible way to do this could be to add an extension type (maybe hyper::client::connect::Fresh? Like CURLOPT_FRESH_CONNECT?) that you add to the request extensions, and if present, the client skips checking the pool.

But it may help to hear from anyone else who has needed a similar feature.

@jdisanti
Copy link

jdisanti commented Sep 7, 2022

I like the idea of having hyper::client::connect::Fresh. Usage could look something like:

use hyper::client::connect::Fresh;

let req = Request::builder()
    .method("GET")
    .uri("https://example.com")
    .extension(Fresh)
    .body(())?;
let resp = client.request(req).await?;

This would work for our use-case where we need a new connection on retry since we can just add the Fresh extension as part of our Tower retry policy.

I'm not really sure how this would carry over to hyper 1.0 yet though. From reading the code, it looks like the connection pool was moved to hyper-util, but I can't find any client code that actually references it.

@tjandra-amzn
Copy link

This feature would be really helpful to my team too - one thing we've seen is that we can have stale connections that just don't work anymore or the host they're connected to on the backend had some problem. It'd be nice if the Fresh extension didn't just bypass the lookup in the conn-pool but was a way to force the expiration and replacement of the existing pool in the connection.

@jdisanti
Copy link

@tjandra-amzn - That seems like a good idea to me.

I talked to @rcoh about this, and he had an interesting idea about having the PoolClient place an extension in responses that identifies the Key in the connection pool used to produce that response. Then a method could be added to client to evict the connections for a given response.

Thinking about that more, in the case where DNS starts responding with a new IP, I wonder if it would be better to evict only the connection that failed rather than all the connections with the same key, as there could be some newer connections in the pool that are still good.

To accomplish that, each PoolClient's connection info could add an extension value unique to that connection to each response so that the individual connection from the pool could be identified and evicted. Or, perhaps this extension value could be the resolved IP address so that all the connections for that given IP could be evicted?

@seanmonstar, what do you think about this approach?

@tjandra-amzn
Copy link

@jdisanti That sounds like a good idea to me - we probably want to be invalidating as few connections as possible to prevent concerted storms of new connection attempts if the backing endpoint is partially available which the proposal would help with.

@seanmonstar
Copy link
Member

I think the general approach sounds reasonable. It kind of sounds like 2 separate (though related) features. So, it's probably easier to review and merge them separately. But I'm excited at the notion, making the pool more configurable/controllable is part of why I wanted to move it into hyper-util at first. There's lots of things to explore, and it shouldn't be hindered by "but should that API be frozen in place?"

rcoh added a commit to rcoh/hyper that referenced this issue Feb 13, 2023
Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of hyperium#2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
rcoh added a commit to rcoh/hyper that referenced this issue Feb 13, 2023
Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of hyperium#2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
rcoh added a commit to rcoh/hyper that referenced this issue Feb 13, 2023
Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of hyperium#2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
rcoh added a commit to rcoh/hyper that referenced this issue Feb 13, 2023
Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of hyperium#2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
rcoh added a commit to rcoh/hyper that referenced this issue Feb 14, 2023
Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of hyperium#2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
rcoh added a commit to rcoh/hyper that referenced this issue Feb 21, 2023
Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of hyperium#2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
seanmonstar pushed a commit that referenced this issue Feb 22, 2023
Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of #2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
oddgrd pushed a commit to oddgrd/hyper that referenced this issue Mar 7, 2023
)

Add `capture_connection` functionality. This allows callers to retrieve the `Connected` struct of the connection that was used internally by Hyper. This is in service of hyperium#2605.

Although this uses `http::Extensions` under the hood, the API exposed explicitly hides that detail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-client Area: client. B-rfc Blocked: More comments would be useful in determine next steps. C-feature Category: feature. This is adding a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants