bitreq: Evict dead entries from async connection pool#564
Open
tnull wants to merge 3 commits intorust-bitcoin:masterfrom
Open
bitreq: Evict dead entries from async connection pool#564tnull wants to merge 3 commits intorust-bitcoin:masterfrom
bitreq: Evict dead entries from async connection pool#564tnull wants to merge 3 commits intorust-bitcoin:masterfrom
Conversation
Expose a `pub(crate)` view of whether an `AsyncConnection` is still healthy and, if so, until when. `None` means the inner `next_request_id` has been poisoned (every failure path in `send` already sets this); `Some(t)` is the current `socket_new_requests_timeout`, which `send` already refreshes from the server's `Keep-Alive: timeout=N` header. No behavioural change; groundwork for the pool to check entry validity without duplicating tracking. Co-Authored-By: HAL 9000
Validate pool entries on acquire and clean up after every send: - Remove entries whose keep-alive deadline has passed or whose inner state has been poisoned, rather than blindly cloning the `Arc`. - Evict on send failure, `Connection: close`, or malformed `Keep-Alive`. - Refresh LRU position on a cache hit. Pre-send insertion of a fresh `Arc` on a miss is kept so concurrent callers arriving during the first round-trip can share the socket for pipelining; `reusable_until` and the post-send eviction together ensure they never observe a known-dead `Arc`. Co-Authored-By: HAL 9000
Drives `a, b, a, c, a` through a `Client` with capacity 2, backed by three `tokio::net::TcpListener` servers so each URL resolves to a distinct `ConnectionKey`. On the fix, step 3 promotes `a` to most-recent, step 4's c-insert evicts `b`, and step 5 reuses the warm `a` entry — three TCP accepts. Pre-fix, `a` stays at the front of the LRU queue and gets evicted at step 4 instead — four accepts. Covers one regression only: the LRU refresh on hit. The other architectural improvement on this branch — explicit pool-layer eviction on failure / `Connection: close` — produces the same externally-observable behaviour as the pre-fix code because `AsyncConnection::send`'s interior `retry_new_connection!` machinery compensates by swapping poisoned inner state on the next use. That change is defended on architectural grounds (explicit pool-layer logic instead of reliance on the inner retry) rather than via a black-box regression test. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2026-04-bitreq-async-pool-fix
branch
from
04a56f3 to
708d9c3
Compare
This was referenced Apr 22, 2026
storopoli
suggested changes
Apr 22, 2026
| /// Removes any pool entry for `key`. No-op if the slot is already empty. | ||
| fn evict(&self, key: &ConnectionKey) { | ||
| let mut state = self.r#async.lock().unwrap(); | ||
| state.connections.remove(key); |
Contributor
There was a problem hiding this comment.
I think this should evict by identity, not just by ConnectionKey.
There is a race where an older poisoned Arc can finish later and remove a newer healthy entry for the same key:
- tasks A and B are both using the old pooled connection for
K - A fails and marks the old connection dead
- task C opens and inserts a fresh connection for
K - B reaches this cleanup path later and
remove(key)drops C's fresh entry
That doesn't break in-flight requests, but it does create avoidable reconnect churn under load. I think evict needs to compare the currently pooled Arc against the one that just finished and only remove on Arc::ptr_eq.
Permalinks:
TheBlueMatt
reviewed
Apr 24, 2026
Member
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I'm not sure I'm understanding the fix here - if we hit a dead connection when we go to request we should fix the Connection, not build a new one in the Client?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #562.
Validate pool entries on acquire and clean up after every send:
state has been poisoned, rather than blindly cloning the
Arc.Connection: close, or malformedKeep-Alive.Pre-send insertion of a fresh
Arcon a miss is kept so concurrentcallers arriving during the first round-trip can share the socket for
pipelining;
reusable_untiland the post-send eviction togetherensure they never observe a known-dead
Arc.