Arm request timeouts on an event loop - #2313
Open
pavel-ptashyts wants to merge 1 commit into
Open
Conversation
A hashed wheel fires on the first tick at or after a deadline, so a deadline near or below the tick duration is rounded up to it, and one timer thread carries every expiry for the whole client. Both hurt short deadlines: a tick is a large fraction of the budget, and a burst of expiries has no headroom to absorb. Measured over 2000 timeouts armed as one burst on Netty 4.2.16, a 20 ms deadline overshot by a mean of 2.7 ms and a p99 of 5 ms on a 5 ms wheel, 1.3/2 ms on a 1 ms wheel, and 0/0 ms scheduled on an event loop, which derives its select timeout from the nearest deadline and so rounds nothing. Add isUseEventLoopTimeouts(), off by default, which arms the request and read timeouts on an event loop instead. On the pooled path the channel is already in hand, so its own loop is used and the timeout expires on the thread that would have to close it. On the connect path there is no channel yet, deliberately, so that the timeout also bounds address resolution and the connect: any loop will do there, since what the wheel costs is a single thread and a rounded-up tick rather than the identity of the thread. Deliberately not a wheel per event loop, which is how the Aerospike client solves this. A wheel arms in O(1) against O(log n) for a deadline queue, but at a few thousand timeouts per loop that is a dozen comparisons, while the quantization it reintroduces costs milliseconds on a 20 ms budget; it also has to be ticked forever, waking every loop even with nothing armed. Aerospike wrote its own wheel because its EventLoop abstracts over NIO, Netty and direct NIO and needed one timer; AHC is Netty-only and gets a per-loop deadline queue for free. Arming allocates nothing beyond what the scheduler needs: the cancellation handle lives on the task, and the existing done flag stands in for the scheduler's already-expired flag, so no per-timeout wrapper is required. Left off by default because the expiry, and therefore whatever the caller chained onto the response future, then runs on an I/O thread. Blocking one stalls every connection it serves. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Problem
Request and read timeouts are armed on the client's
HashedWheelTimer. That has twoproperties that only show up on short deadlines:
deadline near or below
hashedWheelTimerTickDurationis rounded up to it.HashedWheelTimer'sdefault
taskExecutorisImmediateExecutor, so each expiry runs inline on the wheelthread — including
future.completeExceptionally(...)and therefore whatever the callerchained onto the response future.
On a one-second budget the first costs 0.3% and nobody notices. On a budget of tens of
milliseconds a tick is a large fraction of it, and a burst of expiries has no headroom to
absorb before the wheel starts running late.
Measured
2000 timeouts armed as one burst on Netty 4.2.16, JDK 17, tasks doing nothing but
recording their own lag. This is the floor; real work on the firing thread only adds to it.
EventLoop.scheduleEventLoop.scheduleAn event loop shows zero overshoot because it schedules by deadline and derives its own
select()timeout from the nearest one. There is no quantum to round to.This was a throwaway probe rather than JMH —
client/src/jmh/javais not currently wiredinto the build, so its benchmarks do not compile. Happy to add a proper benchmark if that
is fixed first, or as part of this.
Change
AsyncHttpClientConfig#isUseEventLoopTimeouts(), off by default, arms the request andread timeouts on an event loop instead of the timer.
so its own loop is used and the timeout expires on the thread that would have to close
it. On the connect path there is no channel yet — deliberately, so the timeout also bounds
address resolution and the connect — and any loop will do, because what the wheel costs is
a single thread and a rounded-up tick rather than the identity of the thread.
in a wrapper, and the existing
doneflag stands in for the scheduler's already-expiredflag, which the two schedulers spell differently.
isShuttingDown()can return false andschedulerejectimmediately after. Netty answers a rejected timeout with a logged warning rather than an
exception, which would leave the exchange with nothing to end it, so a rejection falls
back to the timer.
Off by default because the expiry — and so whatever the caller chained onto the future —
then runs on an I/O thread, and blocking one stalls every connection it serves. The javadoc
says so and points callers at
handleAsync.Why not a wheel per event loop
That is how the Aerospike client solves the same problem:
EventLoopBaseowns aHashedWheelTimerthat is aRunnablethe loop ticks itself. Deliberately not copied here.A wheel arms in O(1) against O(log n) for a deadline queue, but at a few thousand timeouts
per loop that is a dozen comparisons, while the quantization it reintroduces costs
milliseconds on a 20 ms budget — the third row above is the whole point. A wheel also has to
be ticked forever, waking every loop even with nothing armed. Aerospike wrote its own because
its
EventLoopabstracts over NIO, Netty and direct NIO and needed one timer; AHC isNetty-only and gets a per-loop deadline queue for free.
API compatibility
No exception needed —
revapipasses as-is. The change is additive: the existingTimeoutsHolderconstructor is kept and delegates,TimeoutTimerTaskgainsRunnablewithout losing anything, and nothing is removed.
One place where narrowing was avoided on purpose:
TimerTask#rundeclaresthrows ExceptionandRunnable#rundoes not. Rather than re-declaring the abstract method withoutthe throws clause, which would break an external subclass that declares it, the
Runnableentry point catches and logs.
Tests
EventLoopTimeoutTestasserts the switch itself rather than its side effects: which threadonThrowableis called on. With the flag on it is one of the client's I/O threads; with itoff it is the timer thread. Both go through a real request against an endpoint that answers
well after the deadline.
Verification
mvnw clean verify— BUILD SUCCESS, 1466 tests, 0 failures, 0 errors, 21 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.
Claude Code on behalf of @pavel-ptashyts
🤖 Generated with Claude Code