Conversation
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
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.
Running this gem in production behind a payment gateway, we saw a steady stream
of
ConnectionResetErrorandReadTimeoutErrorthat didn't correspond toanything wrong upstream. Tracking them down surfaced four defects in connection
pooling. One commit each, reviewable independently.
1. A connection probe could raise out of the pool
PoolEntry#healthy?asks the socket whether the peer is still there. When a peerresets a connection,
eof?raisesErrno::ECONNRESETrather than returningtrue, and nothing caught it. That escaped out ofacquire(as an error on arequest not yet sent), out of
release(replacing an already successful responsefrom inside
ensure), and out ofclean_pool_unsafe!— which reaps viato_clean?, so one dead connection broke cleanup for the whole pool and failedrequests to unrelated healthy hosts.
The rescue is scoped to what a socket probe can raise.
Timeout::Errorisexcluded on purpose: Rack::Timeout raises it asynchronously and swallowing it
would defeat the caller's own timeout.
2. Dead connections weren't reaped while any connection was in flight
Cleanup ran only when every entry was reapable (
all?), so one in-flightconnection blocked removal of every dead one beside it and a full pool could
never make room. Separately,
release!never updated@last_used, so idle timewas measured from when a request started — any request slower than
:idle_ttlleft its connection already expired on return.
3. Stale keep-alive connections had no recovery path
keep_alive_timeoutwas pinned at 30 days, disabling Net::HTTP's own stalenesscheck, and
max_retrieswas 0. It's now derived from the pool's:idle_ttlsoboth layers agree.
max_retriesstays 0 deliberately — Net::HTTP's retry list includesNet::ReadTimeout, which would double the worst case latency of a slow request.Instead
with_clientreplays once, guarded: only connections that camestarted out of the pool, only once, only idempotent methods, and never when
the request carries files (Net::HTTP re-encodes multipart bodies from source
IOs that are at EOF after the first attempt — replaying would ship a truncated
body with a matching
Content-Lengthand the server would accept it with a 200).Every error in the retry list fails immediately, so the replay costs ~1ms.
4. Seven network faults escaped as raw Ruby exceptions
rescue MintHttp::Errornever caught them:Errno::ECONNABORTEDConnectionAbortedError— declared but never raised until nowErrno::ENETRESETConnectionResetErrorNet::HTTPBadResponseConnectionIoError— how a desynced keep-alive connection presentsETIMEDOUT,EHOSTDOWN,ENETDOWN,EADDRNOTAVAILConnectionErrorPool exhaustion raised a bare
RuntimeError, bypassing everyrescue MintHttp::.Now
PoolTimeoutError, placed underErrorrather thanConnectionError— asaturated pool isn't a network fault and shouldn't be retried as one.
Tests
13 new tests across 4 files, in the existing local-
TCPServerstyle. Every onewas checked against
77118f2to confirm it fails without the fix. Three assertthat something doesn't happen (fresh connections aren't replayed, requests with
files are never replayed, probe bugs aren't swallowed) — those pass on the old
code too and guard the guards rather than the fixes.
Suite: 25 runs, 73 assertions, 0 failures (was 12/39).
New fake servers bind to port
0instead of hardcoded ports — nine across fourfiles would otherwise collide. Happy to match the existing convention if you'd
rather.
Note for reviewers
PoolTimeoutErroris new public API, and code rescuing thoseErrnoclassesdirectly around a MintHttp call will stop matching them — probably worth a minor
bump. No version bump included; that looked like your call.