Bound an exchange by one request timeout - #2314
Conversation
TimeoutsHolder anchors the request deadline on its own construction, and a redirect, a retry and an auth replay each build a new one for the same future. Every hop therefore starts the budget again: with maxRedirects=5 a chain can legitimately run for six times the configured requestTimeout. The javadoc claims requestTimeout is the maximum time until the response is completed, which is not what happens. Add isUseAbsoluteRequestDeadline(), off by default, which anchors the deadline on when the exchange was submitted instead, so a later hop gets whatever is left of the budget rather than a fresh one. Off by default because turning it on shortens exchanges that rely on the per-attempt behaviour; the getRequestTimeout() javadoc now describes what actually happens and points at the flag either way. Settable per request as well as per client, following the followRedirect pattern: a nullable Boolean on Request that overrides the config value. Resolved once, in newNettyResponseFuture, and kept on the NettyResponseFuture rather than read from the request per hop. The first attempt put it on Request alone, and the two override tests failed in opposite directions because Redirect30xInterceptor rebuilds the request for the next hop from a hand-picked set of fields: the override was dropped mid-exchange and the config value took over. Anything carried only on the request has that problem, so the flag lives on the exchange, which is also what it describes. A redirect target cannot change it, which is right - the budget belongs to the caller. DefaultRequest keeps its existing public constructor, delegating to a new one that takes the flag as a trailing argument. Inserting the parameter beside followRedirect instead was a binary-incompatible change to a public constructor, which revapi correctly rejected. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…quest-deadline-standalone # Conflicts: # client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java # client/src/main/java/org/asynchttpclient/netty/timeout/TimeoutsHolder.java
Review round one on the absolute deadline. Clamping a spent deadline to zero only delayed the abort. The next hop still took a connection permit, took a connection and wrote the request, and the timeout arrived a tick later - so a 307 put its body on the redirect target while the caller was handed a TimeoutException that reads as though nothing had been sent. sendNextRequest, which every redirect, auth replay and retry funnels through, now fails the exchange before the write when the deadline has passed. Worse than reported, as it turns out: on a coarse wheel the exchange did not merely abort late, it ran both hops and succeeded, the deadline exceeded and nobody told. The anchor was getStart(), which is currentTimeMillis. A per-attempt timeout can only be distorted by a clock step for the length of one hop; an anchor spanning a whole exchange carries the step to every hop after it, so a correction back would hand a later hop a budget it never had and one forward would abort it on a healthy connection. The future now records System.nanoTime() at submission and the budget is netted off that, leaving wall clock only where main already had it, per hop. The comment justifying the clamp said the task still cancels its read-timeout sibling. There is no sibling at that point: the read timeout is armed after the write. The reason is simply that a scheduler has no use for a negative delay and the task has to run, running being what fails the exchange. DefaultRequest's widened constructor is package private. Public, it would be pinned by revapi at twenty-seven arguments, the next per-request option would make it twenty-eight, and the two parameter lists would have to be kept in step by hand with a tail that is all reference types for the compiler to confuse. RequestBuilderBase#build is the only caller. Two fields were being dropped by hand-copied lists, which is the same fault this change exists to fix. Redirect30xInterceptor rebuilds the next request from a hand-picked set and carried neither the read timeout - reverting a per-request value to the config default on every hop after the first - nor the deadline flag, which left the Request disagreeing with the behaviour a filter or a signature calculator would read off it. RequestBuilderBase's signature-calculator copy block dropped the read timeout the same way. The interface default returns false rather than reading the property, as every other option on it does; the javadoc now says so, so that a custom config setting the property and getting nothing is documented rather than surprising. Three of the tests asserted only that nothing was thrown, which a dropped Location header or redirects turned off would have satisfied having run a single hop. They assert the final status and which hop it came from. The five were also all wall clock, so TimeoutsHolderTest covers the anchor and the clamp directly: what a second holder makes of the same exchange is the whole difference between the two modes. AsyncHttpClientDefaultsTest was asserting no default for this option, nor for the event-loop one merged alongside it, and now does both. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 1 addressed, and #2313 is merged in - as a merge rather than a rebase, since the branch is pushed and The two that were bugs rather than polish:
The rest: Tests: the three cases that only checked "nothing threw" now assert the final status and which hop it came from;
|
Review round two on the absolute deadline. The check that refuses a hop with nothing left to spend was in sendNextRequest, which only continuations pass through. A first attempt goes straight to sendRequest, and under ROUND_ROBIN the up-front resolve asks for no timeout at all, so a slow resolver could eat the whole budget before any holder existed and a pooled hit would then arm at zero and write anyway. The check has moved into scheduleRequestTimeout, which every attempt passes through, first or otherwise, and which is the last point before the request is written. It returns whether the attempt may go ahead; the four call sites stop when it may not. Asking the holder was the wrong question anyway: the first attempt has no holder to ask. The budget is now a static on TimeoutsHolder, taken off the future, which every caller has in hand before an attempt of its own exists. Long.MAX_VALUE stands for an exchange that is not bounded as a whole, so a per-attempt timeout needs no special case at the call site. Resolving the configured timeout moved there with it, so the constructor and the budget no longer resolve it separately. Three comments described the change rather than the code, which AGENTS.md asks us not to and which will not read well in a year: the two on the fields Redirect30xInterceptor was dropping, and the one on DefaultRequest's package-private constructor. The comment on requestTimeoutMillisTime claimed isDeadlinePassed depended on it staying negative; only startReadTimeout does. And the javadoc for the budget had been left sitting on the test accessor by an earlier edit. One test asserted nothing. isDeadlinePassed answered on its first conjunct for a per-attempt exchange, so the arithmetic it was meant to cover never ran. It asserts on the deadline the holder computes instead: that a hop is handed the configured timeout of its own however long the exchange has already run. Carrying the read timeout across a redirect changes behaviour outside this flag - a short per-request read timeout was reverting to the config default on every hop after the first and now does not - and is called out in the pull request for the release notes. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 2 addressed. The one that mattered: the deadline check was only gating continuations, and asking a The rest: the budget's javadoc is back on the budget rather than on the test accessor an earlier edit of mine slipped in front of it; the comment about the deadline being left negative now names what actually depends on it; three comments that described the change rather than the code are rewritten, Carrying the read timeout across a redirect changes behaviour outside this flag, so it now has its own heading in the description above rather than a line in the change list. There is no release notes file in the repo; say where you would like it and I will put it there.
|
2 participants
Problem
TimeoutsHolderanchors the request deadline on its own construction:A redirect, a retry and an auth replay all continue the same exchange on the same
NettyResponseFuture, but each builds a new holder for it. Every hop therefore starts thebudget again, so with
maxRedirects=5a chain can legitimately run for six times theconfigured
requestTimeout. Nothing carries an absolute deadline across hops:NettyResponseFuture#getStart()exists but is only read for a diagnosticagein a log line.The
getRequestTimeout()javadoc says it is "the maximum time an AsyncHttpClient waits untilthe response is completed", which is not what happens.
Change
AsyncHttpClientConfig#isUseAbsoluteRequestDeadline(), off by default, anchors thedeadline on when the exchange was submitted instead, so a later hop gets whatever is left of
the budget rather than a fresh one.
Off by default because turning it on shortens exchanges that rely on the per-attempt
behaviour, which is a behaviour change even if the current one contradicts the docs. The
getRequestTimeout()javadoc now describes what actually happens and points at the flag, soit stops being wrong either way.
Settable per request as well as per client, following the existing
followRedirectpattern: a nullable
BooleanonRequestthat overrides the config value.Where the flag lives, and why not on the request
It is resolved once, in
newNettyResponseFuture, and kept on theNettyResponseFuture.The first attempt kept it only on
Requestand the two override tests failed in oppositedirections.
Redirect30xInterceptorrebuilds the request for the next hop from a hand-pickedset of fields, so the override was silently dropped mid-exchange and the config value took
over - precisely in the case the setting exists for. Anything carried only on the request has
that problem, and every future site that rebuilds a request would have to remember it.
Keeping it on the exchange also says the right thing: the deadline describes the exchange, and
a redirect target cannot change it, because the budget belongs to the caller.
Review round 1
Two of the nine were bugs rather than polish, and both are worth reading before the rest:
An attempt the deadline has no time for is no longer sent. Clamping a spent deadline to zero
only delayed the abort: the attempt still took a connection permit, took a connection and wrote
the request, and the timeout arrived a tick later - so a 307 put its body on the redirect target
while the caller was handed a
TimeoutExceptionthat reads as though nothing had been sent.scheduleRequestTimeoutnow fails the exchange before the write. Every attempt passes throughit, first or otherwise, and it is the last point before the request goes out; it returns whether
the attempt may go ahead and its four call sites stop when it may not. Disabling that guard
while keeping the new test shows the old behaviour was not even a late abort: on a wheel too
coarse to expire the exchange in time, it ran both hops and completed successfully, the
deadline exceeded and nothing reported.
The budget is a static on
TimeoutsHolder, asked of the future rather than of a holder, becausea first attempt has no holder to ask - and under
ROUND_ROBINthe up-front resolve asks for notimeout at all, so a slow resolver could spend the whole budget before one existed.
Long.MAX_VALUEstands for an exchange that is not bounded as a whole, so a per-attempt timeoutneeds no special case at any call site.
The anchor is monotonic. It was
getStart(), which iscurrentTimeMillis. A per-attempttimeout can only be distorted by a clock step for the length of one hop; an anchor spanning a
whole exchange carries the step to every hop after it, so a correction backwards hands a later
hop a budget it never had and one forwards aborts it on a healthy connection. The future now
records
System.nanoTime()at submission and the budget is netted off that. Wall clock is leftonly where main already had it -
requestTimeoutMillisTimeis still recomputed per hop, for theread-timeout comparison.
Also from the review: two fields were being dropped by hand-copied lists, which is the same
fault this change exists to fix.
Redirect30xInterceptorrebuilds the next request from ahand-picked set and carried neither the read timeout - reverting a per-request value to the
config default on every hop after the first - nor the deadline flag, which left the
Requestdisagreeing with what the exchange was being held to.
RequestBuilderBase'ssignature-calculator copy block dropped the read timeout the same way. And the comment
justifying the clamp claimed the task still cancels its read-timeout sibling; there is no
sibling at that point, the read timeout being armed after the write.
Behaviour change outside the flag
Carrying the read timeout across a redirect applies whether or not anyone turns the deadline on.
A caller with a short per-request read timeout and
followRedirectwas getting the configdefault - 60 s unless configured otherwise - on every hop after the first, and now gets the
value they set. That is a fix, but it is a visible one and belongs in the release notes. There is
no release notes file in the repo, so it is called out here.
API compatibility
revapipasses with no entries.DefaultRequestkeeps its existing public constructor, andthe widened one taking the flag is package private: public, it would be pinned by revapi at
twenty-seven arguments, the next per-request option would make it twenty-eight, and the two
parameter lists would have to be kept in step by hand with a tail that is all reference types
for the compiler to confuse.
RequestBuilderBase#buildis the only caller.Request#getUseAbsoluteRequestDeadline()is adefaultmethod returning null, so existingimplementations are unaffected.
AsyncHttpClientConfig#isUseAbsoluteRequestDeadline()returns a literal rather than readingorg.asynchttpclient.useAbsoluteRequestDeadline, as every other option on that interface does.The javadoc now says so, so that an implementation setting the property and getting nothing is
documented rather than surprising.
Tests
AbsoluteRequestDeadlineTestruns two hops of 400 ms against a 600 ms budget, so each hop fitson its own and the pair does not. Six cases:
The cases that pass assert the final 200 and which hop it came from, not merely that nothing was
thrown - a dropped
Locationheader orfollowRedirectturned off would satisfy that having runa single hop. The last case pins its client to a wheel too coarse to expire anything, which is
the only way to land reliably in the window where the deadline has passed and the timeout has not
yet run; it was checked by mutation rather than by assumption.
TimeoutsHolderTestcovers the anchor and the clamp without a wall clock. Given no timer and norequest sender, a holder computes its deadline and arms nothing, so what a second holder makes of
the same exchange is the whole of the difference between the two modes: the anchor holding across
hops, a per-attempt timeout starting a fresh budget, a spent deadline reporting itself as passed,
and a per-attempt exchange never reporting that however long it has run.
AsyncHttpClientDefaultsTestasserts this default and the property that sets it, along with theevent-loop one merged alongside, which had the same gap.
Timing-based, so
@RepeatedIfExceptionsTest, matching the neighbouring timeout tests.Verification
mvnw clean verify- BUILD SUCCESS, 1485 tests, 0 failures, 0 errors, 26 skipped. ErrorProne, NullAway and Revapi all clean.
Caveat on the testing gate:
AGENTS.mdrequires the build to run on JDK 11 and no JDK 11 isinstalled on this machine, so it was run on JDK 17 (also in the CI matrix). The JDK 11 leg
of CI on this PR is the real gate.
Relationship to #2313
Resolved: #2313 merged first and
mainis merged in here. A merge rather than a rebase, sincethe branch is published and
AGENTS.mdrules out force-pushing a shared one.The conflict was the four lines of the
TimeoutsHolderconstructor both changes touch, plus thetwo config methods landing in the same place.
start()now arms with the remaining budget whenthe deadline is absolute and with the configured duration when it is per attempt, which is what
#2313 left it doing.
Noticed while running the suite
Two timing tests are flaky under load and unrelated to this change, mentioned only so a red
run is not mistaken for this PR:
SemaphoreTest.checkAcquireTime(three methods) allow 400 ms for a 100 ms timeout and use@RepeatedTest(10), which does not retry, unlike thecheckReleasetests beside them. Thisfailed one cell of thirteen on Arm request timeouts on an event loop #2313's CI (macOS, JDK 21) at 420 ms.
NettyRequestThrottleTimeoutTest.testRequestTimeouttakes ~31 s against a 30 s latch even onan idle machine, and releases its throttle permit only from
onThrowable, so a single requestcompleting instead of timing out deadlocks the remaining threads.
Happy to send a separate PR for both.
Claude Code on behalf of @pavel-ptashyts
🤖 Generated with Claude Code