Skip to content

pool-ng#230

Closed
seanmonstar wants to merge 2 commits into
masterfrom
pool-ng
Closed

pool-ng#230
seanmonstar wants to merge 2 commits into
masterfrom
pool-ng

Conversation

@seanmonstar

Copy link
Copy Markdown
Member

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

@seanmonstar

This comment was marked as outdated.

2 similar comments
@seanmonstar

This comment was marked as outdated.

@seanmonstar

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@seanmonstar

This comment was marked as outdated.

@seanmonstar

This comment was marked as outdated.

@seanmonstar

Copy link
Copy Markdown
Member Author

/rustdoc-preview

@github-actions

Copy link
Copy Markdown

📝 Rustdoc preview for this PR: View docs

@seanmonstar
seanmonstar force-pushed the pool-ng branch 2 times, most recently from 778d49d to 7b668a6 Compare September 19, 2025 20:38
@seanmonstar

Copy link
Copy Markdown
Member Author

/rustdoc-preview

@github-actions

Copy link
Copy Markdown

📝 Rustdoc preview for this PR: View docs

@sjcobb2022 sjcobb2022 left a comment

Copy link
Copy Markdown

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

Comment thread examples/client.rs
Comment thread examples/client.rs
Comment thread examples/client.rs
Comment thread examples/client.rs
Comment thread 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
Copy Markdown

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.

Comment thread src/client/pool/map.rs
let mut pool = super::Map::builder().keys(|_| "a").values(|_| "b").build();
pool.service(&"hello");
}
}

Copy link
Copy Markdown

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");
    }

Comment thread src/client/pool/singleton.rs Outdated
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
Copy Markdown

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.

Comment thread src/client/pool/map.rs
Comment thread src/client/pool/map.rs
T::Key: Eq + std::hash::Hash,
{
/// Get a service after extracting the key from `req`.
pub fn service(&mut self, req: &Req) -> &mut T::Service {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it still be worth implementing Service for this, which just calls the underlying method? Means we have a "consistent" API for all the pool methods.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So far I still think it's proper to not provide the impl. Wouldn't be a problem to eventually add one if so needed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeh sounds good.

@sjcobb2022 sjcobb2022 Nov 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, if we want to store Map, it would be much nicer if we had Service for it, since that means we can name the type that we can name the type of the returned service.

e.g. we can store cache, singleton, and negotiate because they can be abstracted away into Services over children services, but since target is private we can't really name it.

@seanmonstar
seanmonstar force-pushed the pool-ng branch 3 times, most recently from 20769ab to 63227af Compare November 11, 2025 19:23
@seanmonstar
seanmonstar force-pushed the pool-ng branch 2 times, most recently from 8411023 to 09a85d0 Compare November 11, 2025 22:42
@seanmonstar

Copy link
Copy Markdown
Member Author

/rustdoc-preview

@github-actions

Copy link
Copy Markdown

📝 Rustdoc preview for this PR: View docs

@seanmonstar
seanmonstar force-pushed the pool-ng branch 5 times, most recently from 16e4b8b to e829e93 Compare November 18, 2025 22:00
@seanmonstar

Copy link
Copy Markdown
Member Author

Alright, each piece worth keeping has been merged individually. This PR now longer has any use, so closing it down.

@seanmonstar seanmonstar closed this Dec 2, 2025
@seanmonstar
seanmonstar deleted the pool-ng branch December 2, 2025 20:22
github-actions Bot pushed a commit that referenced this pull request Dec 2, 2025
@sjcobb2022

Copy link
Copy Markdown

@seanmonstar it did have the client example which we didn't have elsewhere

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.

2 participants