Skip to content

Fix reused_stream losing healthy pooled connections (race with idle poller) - #1005

Open
sinhaparth5 wants to merge 1 commit into
cloudflare:mainfrom
sinhaparth5:fix/reused-stream-try-unwrap-race
Open

Fix reused_stream losing healthy pooled connections (race with idle poller)#1005
sinhaparth5 wants to merge 1 commit into
cloudflare:mainfrom
sinhaparth5:fix/reused-stream-try-unwrap-race

Conversation

@sinhaparth5

Copy link
Copy Markdown

Fixes #998.

Root cause

TransportConnector::reused_stream waits for the idle poller's OwnedMutexGuard to be released (by briefly locking the pooled stream's mutex), then calls Arc::try_unwrap, assuming the strong count is back down to 1.

That assumption races with tokio::sync::OwnedMutexGuard::drop, which releases the semaphore permit before it drops its own Arc<Mutex<T>> clone (the permit release happens inside the Drop impl's body, and the guard's Arc field is only dropped after that body returns). So there's a window where the waiter in reused_stream gets woken and scheduled on another thread before the poller's task has actually finished dropping its Arc clone. If try_unwrap runs in that window, it returns Err, the healthy pooled connection gets thrown away, and the caller dials a new connection instead of reusing it.

Nothing is corrupted or leaked. The only symptom is silently lost keepalive reuse, which on a busy proxy means extra TCP/TLS handshakes on the upstream path under load.

Fix

Stop depending on the Arc strong count. The pooled stream is now wrapped as Arc<Mutex<ReusableStream>>, where ReusableStream(Option<Stream>), and reused_stream holds the lock guard and take()s the stream out of the Option instead of calling Arc::try_unwrap. Holding the mutex guard is enough on its own for exclusivity, regardless of whether the poller's leftover Arc clone has finished dropping yet.

Testing

  • cargo test -p pingora-core --features rustls passes.
  • Reproduced the race on an isolated 8-vCPU GCE VM (torn down afterward): looping connectors::tests::test_connect_uds as 400 fresh processes (fresh multi-threaded runtime per iteration, 8-way parallel, matching the repro methodology in reused_stream can miss a pooled connection when try_unwrap races the idle poller #998) gave 11/400 (2.75%) failures on the parent commit and 0/400 on this fix, all at the same assert!(reused) site described in the issue.

…oller)

TransportConnector::reused_stream waited for the idle poller's
OwnedMutexGuard to drop by briefly taking the stream's lock, then called
Arc::try_unwrap assuming the strong count was back to 1. That races with
tokio's OwnedMutexGuard::drop, which releases the semaphore *before*
dropping its own Arc<Mutex<T>> clone: the waiter can be scheduled and reach
try_unwrap while the poller's clone is still mid-drop. try_unwrap then
returns Err, the connection is discarded, and the caller dials a new one
even though the pooled connection was perfectly healthy.

Fix by not depending on the Arc strong count at all: wrap the pooled stream
in a new ReusableStream(Option<Stream>) and take() it out while still
holding the mutex guard. Exclusive access to the lock is sufficient on its
own to make the take() race-free, regardless of when the poller's leftover
Arc clone finishes dropping.

Fixes cloudflare#998
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.

reused_stream can miss a pooled connection when try_unwrap races the idle poller

1 participant