hyper-util icon indicating copy to clipboard operation
hyper-util copied to clipboard

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

Open magurotuna opened this issue 1 year ago • 4 comments

This is a prototype of my idea proposed in https://github.com/hyperium/hyper/issues/3727#issuecomment-2273756068.

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 https://github.com/hyperium/hyper/issues/3727

magurotuna avatar Aug 18 '24 14:08 magurotuna

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.

seanmonstar avatar Aug 20 '24 19:08 seanmonstar

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.

nox avatar Sep 03 '24 09:09 nox

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

seanmonstar avatar Sep 03 '24 20:09 seanmonstar

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

magurotuna avatar Sep 04 '24 03:09 magurotuna