hyperium / hyper

An HTTP library for Rust
https://hyper.rs
MIT License
14.42k stars 1.59k forks source link

Allow for generic Keys in connection pooling. #3064

Closed EItanya closed 1 year ago

EItanya commented 1 year ago

Is your feature request related to a problem? Please describe. Currently I wish to use the hyper client for http2 connection pooling, however I need to be able to pool by specific metadata. Specifically the SourceIp/Identity.

However, currently the keys in the connection pooling code are limited to a very specific set of data, specifically the protocol and the authority: https://github.com/hyperium/hyper/blob/d32beb3843179acdaf0e64f3e708b9dbcd0c29e4/src/client/pool.rs#L58

Describe the solution you'd like I understand the the client code is built specifically around pooling with this metadata, but if we allowed the key to be generic, rather than that specific data, it would allow reuse of the pooling code for other client implementations.

I created a branch of hyper-utils where I tested this and mostly got it working, would love additional feedback. Here is a gist containing the pool code: https://gist.github.com/EItanya/61c4a2d67eb194e9891f7d54c2806d9d

The most important part is the addition of a second type argument to pool, namely the key type:

pub(super) struct Pool<T, K: Eq + Hash + Clone + Debug + Unpin> {
    // If the pool is disabled, this is None.
    inner: Option<Arc<Mutex<PoolInner<T, K>>>>,
}

Thereby allowing the caller of this code to decide how the connections should be pooled.

This would require very few changes on the part of the hyper client to implement as well.

Describe alternatives you've considered The only other solution I can think of is writing it from scratch, or manually changing it heavily.

Additional context Add any other context or screenshots about the feature request here.

seanmonstar commented 1 year ago

I agree with the goal. As you've noticed, we've moved the pool out of hyper core, exactly so that we can open up its design a bit. I do think the Pool itself should be generic over keys, and then we can just provide an additional type that joins the generic Pool with the Connect stuff.

Just to add to the discussion, another idea I had was not necessarily a Pool<K: Eq, T>, but rather something like Pool<T, K: MakeKeyFrom<T>>. So, then you extract the key from Request by grabbing the Uri... But then again, the suggestion I had could easily be built on top of the simpler Pool<K, T>.

EItanya commented 1 year ago

Ahhh very interesting, as mentioned I have 90% of the code written, so I could put up a PR in hyper-utils and continue the conversation there?

seanmonstar commented 1 year ago

Sounds good to me! And in case it wasn't clear, my extra idea isn't necessarily what it should be. In fact, it might be better as your idea to begin with.

EItanya commented 1 year ago

Here is the PR in hyper-util https://github.com/hyperium/hyper-util/pull/12

EItanya commented 1 year ago

Hey @seanmonstar, just wanted to double check about this PR to get your thoughts on it: https://github.com/hyperium/hyper-util/pull/12

Michael-J-Ward commented 1 year ago

Does https://github.com/hyperium/hyper-util/pull/16 complete this?

howardjohn commented 1 year ago

Does hyperium/hyper-util#16 complete this?

Yes