pool-ng #230

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

Jump to bottom

Draft

seanmonstar wants to merge 5 commits into master

from pool-ng

Draft

pool-ng #230

seanmonstar wants to merge 5 commits into master from pool-ng

Conversation

@seanmonstar

Copy link

Member

@seanmonstar seanmonstar commented Sep 16, 2025

Just a combined pull request for generating docs previews and some CI testing, look at the separate PRs for the individual pieces.

@seanmonstar seanmonstar force-pushed the pool-ng branch from 4d6bdd7 to 47785bc Compare

September 16, 2025 14:01

@seanmonstar

This comment was marked as outdated.

2 similar comments

@seanmonstar

Copy link

Member Author

seanmonstar commented Sep 16, 2025

/rustdoc-preview

@github-actions

Copy link

github-actions bot commented Sep 16, 2025

📝 Rustdoc preview for this PR: View docs

seanmonstar added 2 commits

September 16, 2025 17:00

@seanmonstar


 feat(pool): add a Singleton pool type

c87b743

@seanmonstar


 feat(pool): add a Cache pooling service

6d7eef9

@seanmonstar seanmonstar force-pushed the pool-ng branch from ded06e9 to 778d49d Compare

September 16, 2025 21:01

seanmonstar added 3 commits

September 16, 2025 17:07

@seanmonstar


 feat(pool): add a Negotiate pooling service

11159bb

@seanmonstar


 feat(pool): add a Map pool service type

d562e10

@seanmonstar


 pools wip

7b668a6

@seanmonstar seanmonstar force-pushed the pool-ng branch from 778d49d to 7b668a6 Compare

September 19, 2025 20:38

@seanmonstar

Copy link

Member Author

seanmonstar commented Sep 19, 2025

/rustdoc-preview

@github-actions

Copy link

github-actions bot commented Sep 19, 2025

📝 Rustdoc preview for this PR: View docs

sjcobb2022

sjcobb2022 reviewed

Oct 13, 2025

View reviewed changes

Copy link

@sjcobb2022 sjcobb2022 left a comment

Choose a reason for hiding this comment

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

Looking great overall. Will test in some crates later on in the week

examples/client.rs

use http_body_util::Empty;

use hyper::Request;

use hyper_util::client::legacy::{connect::HttpConnector, Client};

use hyper_util::client::pool;

Copy link

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

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

nit: rename file to client_pool

examples/client.rs

let mut p = pool::Cache::new(http1).build();

let mut c = p.call(http::Uri::from_static("http://hyper.rs")).await?;

Copy link

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

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

nit: use consistent uri for all calls.

examples/client.rs

let req = Request::builder()

.uri(url)

.body(Empty::<bytes::Bytes>::new())?;

async fn send_nego() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {

Copy link

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

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

nit: send_negotiate

examples/client.rs

for _ in 0..5 {

let mut c = svc

.call(http::Uri::from_static("http://localhost:3000"))

Copy link

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

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

change to hyper.rs (or keep consistent with all other tests)

src/client/pool/map.rs

Comment on lines +3 to +9

//! The map isn't a typical `Service`, but rather stand-alone type that can map

//! requests to a key and service factory. This is because the service is more

//! of a router, and cannot determine which inner service to check for

//! backpressure since it's not know until the request is made.

//!

//! The map implementation allows customization of extracting a key, and how to

//! construct a MakeService for that key.

Copy link

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

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

Potential rewrite:

This map is not a traditional Service; instead, it is a standalone type that allows users to associate incoming keys with service factories. For each request, a key is extracted and used to select or construct a corresponding service.

Because this service acts more like a router, it cannot determine which inner service to check for backpressure ahead of time; the appropriate service is only known once a request has been received.

src/client/pool/map.rs

let mut pool = super::Map::builder().keys(|_| "a").values(|_| "b").build();

pool.service(&"hello");

}

Copy link

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

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

 #[test]
 fn it_returns_same_service_for_same_key() {
 let mut pool = Map::builder()
 .keys(|s: &&str| *s)
 .values(|_| {
 String::from("service")
 })
 .build();
 let service1 = pool.service(&"foo") as *const String;
 let service2 = pool.service(&"foo") as *const String;
 // Should be the same pointer.
 assert_eq!(service1, service2);
 }
 #[test]
 fn it_returns_different_service_for_different_keys() {
 let mut pool = Map::builder()
 .keys(|s: &&str| *s)
 .values(|s| format!("service-for-{}", s))
 .build();
 let service1 = pool.service(&"foo") as *const String;
 let service2 = pool.service(&"bar") as *const String;
 // Should NOT be the same pointer (different key/service)
 assert_ne!(service1, service2);
 // Optionally, check the actual value
 assert_eq!(pool.service(&"foo"), "service-for-foo");
 assert_eq!(pool.service(&"bar"), "service-for-bar");
 }

src/client/pool/singleton.rs

Comment on lines +3 to +8

//! The singleton pool combines a MakeService that should only produce a single

//! active connection. It can bundle all concurrent calls to it, so that only

//! one connection is made. All calls to the singleton will return a clone of

//! the inner service once established.

//!

//! This fits the HTTP/2 case well.

Copy link

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

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

Take it or leave it:

The singleton pool manages a MakeService that should only produce a single active connection.

sjcobb2022

sjcobb2022 reviewed

Oct 20, 2025

View reviewed changes

src/client/pool/map.rs

/// Provide a closure that extracts a pool key for the destination.

pub fn keys<K, KK>(self, keyer: K) -> Builder<Dst, K, S>

where

K: Fn(&Dst) -> KK,

Copy link

@sjcobb2022 sjcobb2022 Oct 20, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

Taking by reference here makes making extracting some types of keys quite hard

I think take a type Dst that implement clone would be better, and clone the type for mapping.

This way we don't need to deal with yucky schenarios like the following:

 let mut map = hyper_util::client::pool::map::Map::builder::<Uri>()
 .keys(|uri| uri.scheme()) // lifetime may not live long enough
 .values(|_| ())
 .build();

Labels

None yet

2 participants

@seanmonstar @sjcobb2022

Uh oh!

Uh oh!

pool-ng #230

Are you sure you want to change the base?

pool-ng #230

Uh oh!

Conversation

@seanmonstar seanmonstar commented Sep 16, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

seanmonstar commented Sep 16, 2025

Uh oh!

github-actions bot commented Sep 16, 2025

Uh oh!

seanmonstar commented Sep 19, 2025

Uh oh!

github-actions bot commented Sep 19, 2025

Uh oh!

@sjcobb2022 sjcobb2022 left a comment

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

@sjcobb2022 sjcobb2022 Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

@sjcobb2022 sjcobb2022 Oct 20, 2025 •

edited

Loading