Apply Net::HTTP cutoff to internal retries - #20
Merged
Merged
Conversation
6 tasks
Net::HTTP#transport_request silently retries idempotent requests (GET, HEAD, PUT, DELETE, OPTIONS, TRACE) up to max_retries (default 1) on transient errors like Net::ReadTimeout, OpenSSL::SSL::SSLError, and Errno::ECONNRESET. The retry path closes the socket and goes through re-apply: the retry inherited the read_timeout set on the original attempt. A deadline of N seconds silently became 2N seconds, with no log line or callback to indicate the retry had happened. Factor the timeout-clamping and checkpoint into a private helper and also call it from begin_transport, which runs once per HTTP attempt including retries. On a retry after the cutoff is exhausted, the checkpoint raises CutoffExceededError; Net::HTTP catches it as a Timeout::Error and re-raises once max_retries is hit. With the default max_retries of 1, the second attempt short-circuits cleanly. As a side effect, the keep-alive connection-reuse path now also applies the cutoff. Previously a request reusing a pooled socket skipped the start patch entirely, since #start is only called when opening a new connection.
justinhoward
force-pushed
the
tighten-net-http-timeout-on-retry
branch
from
May 12, 2026 22:32
f90e5ae to
b671cf8
Compare
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
1 participant
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.
Net::HTTP#transport_request silently retries idempotent requests (GET, HEAD, PUT, DELETE, OPTIONS, TRACE) up to max_retries (default 1) on transient errors like Net::ReadTimeout, OpenSSL::SSL::SSLError, and Errno::ECONNRESET. The retry path closes the socket and goes through re-apply: the retry inherited the read_timeout set on the original attempt. A deadline of N seconds silently became 2N seconds, with no log line or callback to indicate the retry had happened.
Factor the timeout-clamping and checkpoint into a private helper and also call it from begin_transport, which runs once per HTTP attempt including retries. On a retry after the cutoff is exhausted, the checkpoint raises CutoffExceededError; Net::HTTP catches it as a Timeout::Error and re-raises once max_retries is hit. With the default max_retries of 1, the second attempt short-circuits cleanly.
As a side effect, the keep-alive connection-reuse path now also applies the cutoff. Previously a request reusing a pooled socket skipped the start patch entirely, since #start is only called when opening a new connection.