[bitreq]: Add blocking Client with connection pooling#518
[bitreq]: Add blocking Client with connection pooling#518tnull wants to merge 3 commits intorust-bitcoin:masterfrom
Client with connection pooling#518Conversation
oleonardolima
left a comment
oleonardolima
left a comment
There was a problem hiding this comment.
cACK 94c62c3
It looks good so far, though I won't be able to do a in-depth review and testing until early next week.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Hmm, while the code is fine, its really annoying that we're diverging so much between the sync and async code, and even duplicating a decent amount of logic between the sync and sync-connection-reuse logic. Maybe
- for both sync and async we pass a reference to the
Clientin so that theConnectioncan lookup a connection to use on redirect rather than moving that logic out toclient.rs. - Similar for connection timeouts, I'm fine with it being in
Connectionor in theClientbut let's pick one and stick to it.
|
Any progress on this one? |
…antics The previous `capacity` parameter name suggested a hard cap on the number of connections a `Client` may keep open, but it never was one: the cache only bounds *idle* entries that have already been put back. Under any burst of concurrent requests, the cache is bypassed entirely — each in-flight request that does not find a cached connection opens a fresh socket. Only when those requests complete and try to return their connection to the cache does the limit kick in, and surplus entries are simply dropped. This means the cache is inherently racy with respect to capacity: we do not (and this commit does not attempt to) gate concurrent connection establishment, so the number of live sockets is unbounded regardless of what the user passes. Rename the parameter to `max_idle` and spell out this nuance in the type-level docs, so that callers understand the bound applies to the idle cache — not to overall concurrency — and can layer their own semaphore if they need to cap live connections. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2026-02-blocking-client
branch
from
94c62c3 to
f795b7b
Compare
Previously, we added a `Client` allowing for connection reuse behind the `async` feature. Here we do the same for the blocking, i.e., non-`async` path. The client pools are exposed as additive capabilities: the blocking pool is always available when `std` is, and the async pool is available when the `async` feature is enabled. Unlike the async path, the blocking pool keys the cache by `(https, host, port, proxy)` and stores the raw keep-alive stream alongside an expiry `Instant` parsed from the server's `Keep-Alive: timeout=N` header (defaulting to 60s). Stream reuse is gated on both `Connection: keep-alive` and the `BufReader` having no trailing bytes, to avoid corrupting the next response on the socket. Redirect handling (301/302/303/307, including 303 method downgrade) is driven by `Connection::send_pooled`, which consults the provided `&Client` for a pooled connection on each hop rather than duplicating the redirect loop in `client.rs`. Keep-alive header parsing lives in `connection.rs` so that "connection lifetime" concerns stay in one module. Additional divergence between the sync and async paths (pipelining, per-request IDs, dropped-reader tracking in `AsyncConnection`) intentionally remains out of scope. Co-Authored-By: HAL 9000
Exercise the blocking `Client::send` path alongside the existing one-shot `send` / `send_lazy` comparisons, so that every integration test asserts that the pooled and non-pooled blocking paths produce identical responses. The assertion block now runs regardless of the `async` feature, since the blocking client is always available. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2026-02-blocking-client
branch
from
f795b7b to
66a33f6
Compare
Now pushed some updates / rewritten commits. Please let me know if that's more in-line with what you had in mind.
Yes, excuse the delay. |
|
About to release the stack in #560, want this in? Although we don't want to hold it up too much because of the RUSTSEC stuff, right? |
Yeah, would be good to ship this one soon, too, but we should probably not hold up the release for it, given it annoys people that their CI is red constantly. |
|
Yep this can go on the next release. Red CI jobs are annoying 😆 |
| struct ClientState { | ||
| blocking_connections: HashMap<ConnectionKey, PoolEntry>, | ||
| #[cfg(feature = "async")] | ||
| async_connections: HashMap<ConnectionKey, Arc<AsyncConnection>>, |
There was a problem hiding this comment.
It seems surprising that a single Client can do both async and sync pooling but also doesn't reuse the connections across them (we can't, but from an API perspective, it feels very surprising).
|
|
||
| pub(crate) struct PoolEntry { | ||
| pub(crate) stream: HttpStream, | ||
| pub(crate) expires_at: Instant, |
There was a problem hiding this comment.
You didn't address my second comment - currently for async we do the retry/new stream/expiry handling all in Connection. It seems a bit easier to keep it that way (because if we go to use a connection and the connection has dropped, we similarly need to make a new connection just like for async/the timed-out case), but I'm certainly okay with moving it out to Client. We should, however, do the same thing for both sync and async.
| ) -> Result<Response, Error> { | ||
| let mut connection = self; | ||
| loop { | ||
| let (response, recovered, req) = connection.send_and_buffer(request)?; |
There was a problem hiding this comment.
We need to handle if the server dropped the connection between when we did the last request and we started this one like we do in async.
5 participants
Previously, we added a
Clientallowing for connection reuse behind theasyncfeature. Here we do the same for the blocking, i.e., non-asyncpath.