-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
base: master
Are you sure you want to change the base?
pool-ng #230
Conversation
4d6bdd7
to
47785bc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
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.
This comment was marked as outdated.
This comment was marked as outdated.
47785bc
to
79b11b8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
79b11b8
to
ded06e9
Compare
/rustdoc-preview |
📝 Rustdoc preview for this PR: View docs |
ded06e9
to
778d49d
Compare
778d49d
to
7b668a6
Compare
/rustdoc-preview |
📝 Rustdoc preview for this PR: View docs |
There was a problem hiding this 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
use http_body_util::Empty; | ||
use hyper::Request; | ||
use hyper_util::client::legacy::{connect::HttpConnector, Client}; | ||
use hyper_util::client::pool; |
There was a problem hiding this comment.
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
|
||
let mut p = pool::Cache::new(http1).build(); | ||
|
||
let mut c = p.call(http::Uri::from_static("http://hyper.rs")).await?; |
There was a problem hiding this comment.
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.
let req = Request::builder() | ||
.uri(url) | ||
.body(Empty::<bytes::Bytes>::new())?; | ||
async fn send_nego() -> Result<(), Box<dyn std::error::Error + Send + Sync>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: send_negotiate
|
||
for _ in 0..5 { | ||
let mut c = svc | ||
.call(http::Uri::from_static("http://localhost:3000")) |
There was a problem hiding this comment.
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)
//! 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. |
There was a problem hiding this comment.
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.
let mut pool = super::Map::builder().keys(|_| "a").values(|_| "b").build(); | ||
pool.service(&"hello"); | ||
} | ||
} |
There was a problem hiding this comment.
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");
}
//! 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. |
There was a problem hiding this comment.
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.
Just a combined pull request for generating docs previews and some CI testing, look at the separate PRs for the individual pieces.