feat(pool): add a Singleton pool type #226

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

Open

seanmonstar wants to merge 1 commit into master

from pool-singleton

Open

feat(pool): add a Singleton pool type #226

seanmonstar wants to merge 1 commit into master from pool-singleton

Conversation

@seanmonstar

Copy link

Member

@seanmonstar seanmonstar commented Sep 15, 2025

This creates a Singleton pool service which wraps an inner make-service, and queues up all calls while waiting for the one instance to be created, and then hands out clones. This pattern fits HTTP/2 well.

The current implementation mirrors much of how the legacy pool handles connecting with HTTP/2 prior knowledge.

Closes hyperium/hyper#3953

@seanmonstar seanmonstar force-pushed the pool-singleton branch 3 times, most recently from 112d15d to 99e0932 Compare

September 15, 2025 15:08

@seanmonstar


 feat(pool): add a Singleton pool type

c87b743

@seanmonstar seanmonstar force-pushed the pool-singleton branch from 99e0932 to c87b743 Compare

September 16, 2025 21:01

@seanmonstar seanmonstar mentioned this pull request

Sep 25, 2025

Proposal: hyper-util composable pool services hyperium/hyper#3948

Open

cratelyn

cratelyn reviewed

Sep 30, 2025

View reviewed changes

Copy link

Member

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

this is really exciting work @seanmonstar! thanks for your patience in review, it took me a couple days to take a deep read through this.

broadly i believe this is already in tremendous shape. my primary concern is bounding the number of pending callers while waiting for the connection to be established.

besides that, i have a question about how this might be extended in the future to accommodate an Arc<T>-backed inner service. that is mostly a forward-facing design question rather than concern with this initial introduction to the client::pool module.

thanks for breaking this work out into distinct pull requests, and for the illustrative outline and examples in hyperium/hyper#3948.

src/client/pool/singleton.rs

Comment on lines +3 to +6

//! 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.

Copy link

Member

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

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

Suggested change

//! 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 ensures that only one connection is made.

//!

//! The singleton pool wraps a `MakeService<T>` so that it only produces a

//! single [`Service<T>`]. Once that service is instantiated, all successive

//! calls to the `Singleton` pool will return a clone of the inner service.

take-it-or-leave-it, docs nit.

src/client/pool/singleton.rs Show resolved Hide resolved

src/client/pool/singleton.rs

Comment on lines +168 to +170

State::Empty | State::Made(_) => {

// shouldn't happen!

}

Copy link

Member

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

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

it'd be nice if this emitted a tracing::warn! should we ever hit this, ideally behind a feature gate.

Copy link

Member Author

@seanmonstar seanmonstar Oct 7, 2025

Choose a reason for hiding this comment

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

Hm, this really should be more like an extreme bug if it triggers. I could see adding some unreachable!() call, maybe gated by debug assertions. 🤷

Copy link

Member

@cratelyn cratelyn Oct 7, 2025

Choose a reason for hiding this comment

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

oh, that's a very nice idea! debug_assert!(false, "this should not happen") would work well.

src/client/pool/singleton.rs

Comment on lines +94 to +96

State::Made(ref svc) => SingletonFuture::Made {

svc: Some(svc.clone()),

Copy link

Member

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

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

something we may wish to add as a follow-on to this would be a similar "singleton" pool that is backed by an Arc<T>. upon my initial read, i was slightly surprised we weren't Arc<T>'ing the generated Service<T> to already.

as is, this calls mk_svc.call() once, but does result in multiple distinct copies of the M::Response service. an Arc<T>-backed singleton pool would be useful for services that are expensive to Clone.

i don't believe any of this blocks this specific change, but i'm curious to ask where we think it might reasonably live. ideally we could rely on a lot of this infrastructure, since 99% of it will be the same! 😸

src/client/pool/singleton.rs

Comment on lines +22 to +24

/// The singleton wraps an inner service maker, bundling all calls to ensure

/// only one service is created. Once made, it returns clones of the made

/// service.

Copy link

Member

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

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

i'd suggest that we clarify this slightly, and indicate that we lazily instantiate the inner service.

i could imagine an alternative "eager" singleton connection pool that invokes the MakeService immediately, for example.

cratelyn

cratelyn approved these changes

Oct 8, 2025

View reviewed changes

Copy link

Member

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

modulo the note(s) on documentation comments, i think this looks great. i am excited to see this work move forward! nice work 🚀

Labels

None yet

2 participants

@seanmonstar @cratelyn

Uh oh!

Uh oh!

feat(pool): add a Singleton pool type #226

Are you sure you want to change the base?

feat(pool): add a Singleton pool type #226

Uh oh!

Conversation

@seanmonstar seanmonstar commented Sep 15, 2025

Uh oh!

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

Uh oh!

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

@seanmonstar seanmonstar Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

@cratelyn cratelyn Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

@cratelyn cratelyn Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants