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

feat!(client): expose whether a connection is reused from the pool #145

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

magurotuna
Copy link
Contributor

@magurotuna magurotuna commented Aug 18, 2024

This is a prototype of my idea proposed in hyperium/hyper#3727 (comment).

The obvious downside of this approach is that this is a breaking change as the return type of Error::connect_info() is changed from Option<&Connected> to Option<&ErroredConnectInfo>, but this may be good in a sense that not all of the methods that Connected offers make sense in the context of connection error - introducing a dedicated type would allow not only for exposing methods that are relevant, but also for making changes to Connected in the future without worrying about Error::connect_info().


This introduces a new type ErrorConnectInfo that contains information about connection pool as well as transport-related data.

This new type can now be obtained from the Error::connect_info() method, which means that this is a breaking change as the return type from the method has changed. That said, all information that was available on the previous return type is available on the new type through various getter methods.

Closes hyperium/hyper#3727

This introduces a new type `ErrorConnectInfo` that contains information
about connection pool as well as transport-related data. This new type
can now be obtained from the `Error::connect_info()` method, which means
that this is a breaking change as the return type from the method has
changed. That said, all information that was available on the previous
return type is available on the new type through various getter methods.
@seanmonstar
Copy link
Member

Thanks for opening this! It does seem like the right direction to go... @nox, since you added the original type, I'm curious if you have any thoughts about this direction.

I'd probably suggest we should wait on the breaking change (it disrupts the crates providing Connectors), and provide a separate method name, and a deprecation warning on the original.

@nox
Copy link
Contributor

nox commented Sep 3, 2024

I'm not sure I gather why this can't be provided by Connected itself. It's already equipped with a poison system about preventing the use of the connection for other requests after all.

@seanmonstar
Copy link
Member

Yea, that's fair. I'd probably be fine with that. @magurotuna any reason you think that isn't ideal?

@magurotuna
Copy link
Contributor Author

The reason I prefer to introduce a new struct wrapping the existing Connected is primarily because the information on whether a particular connection is a fresh one or reused one is managed by Pooled, not PoolClient. So if Connected had is_reused field, it would result in duplication which is I think not good in general.

Pooled, PoolClient, and Connected hierarchy

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.

hyper-util: expose connection info on whether it's reused from the pool or newly established
3 participants