Commit c1af3a4
Arm request timeouts on an event loop (#2313)
## Problem
Request and read timeouts are armed on the client's `HashedWheelTimer`.
That has two
properties that only show up on short deadlines:
* **A wheel quantizes.** It fires on the first tick at or after the
deadline, so a
deadline near or below `hashedWheelTimerTickDuration` is rounded up to
it.
* **One thread carries every expiry** for the whole client, and
`HashedWheelTimer`'s
default `taskExecutor` is `ImmediateExecutor`, so each expiry runs
inline on the wheel
thread - including `future.completeExceptionally(...)` and therefore
whatever the caller
chained 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.
| instrument | deadline | mean | p50 | p99 | max |
|---|---:|---:|---:|---:|---:|
| wheel, tick 5 ms | 20 ms | +2.7 | +2 | +5 | +5 |
| wheel, tick 1 ms | 20 ms | +1.3 | +1 | +2 | +2 |
| `EventLoop.schedule` | 20 ms | +0.0 | +0 | +0 | +0 |
| wheel, tick 5 ms | 1000 ms | +3.0 | +3 | +3 | +3 |
| wheel, tick 1 ms | 1000 ms | +1.9 | +2 | +2 | +2 |
| `EventLoop.schedule` | 1000 ms | +1.6 | +2 | +2 | +2 |
An 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/java` is
not currently wired
into 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 and
read timeouts on an event loop instead of the timer.
* **Always the channel's own loop.** On the pooled path the channel is
already in hand, so
its 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 - so it is armed on the timer and
moved onto the loop
once the connect succeeds. No other loop is ever used; see the review
round below for why
that matters.
* **Arming allocates nothing extra.** The cancellation handle lives on
the task rather than
in a wrapper, and the existing `done` flag stands in for the scheduler's
already-expired
flag, which the two schedulers spell differently.
* **Shutdown race closed.** `isShuttingDown()` can return false and
`schedule` reject
immediately 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.
* The connection-pool cleaner stays on the timer either way.
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:
`EventLoopBase` owns a
`HashedWheelTimer` that is a `Runnable` the 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 `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.
## Review round 1
Most of the substance of this PR changed in review, so the sections
above describe the
current shape rather than what was first pushed. Two things are worth
calling out here
because they were design errors, not polish:
**The loop is now only ever the channel's own.** It used to come from
`EventLoopGroup#next()`, which is almost never the loop the channel ends
up on:
`initAndRegister` draws from the same chooser, so the two agreed about
one time in N. Every
completion then cancelled an entry on a foreign loop, and until the
original deadline that
entry sat in a queue whose loop it would wake for a request that had
long finished. Drawing
from the chooser also shifted which loops connections land on. The
pooled path has the
channel in hand; the connect path arms on the timer, as before this
branch, and
`NettyConnectListener` moves the timeouts onto the loop once the connect
succeeds, next to
the `attachChannel` that publishes it on the future. That listener
already runs on the
channel's loop, so the move costs a same-thread schedule and no wakeup.
**Arming left the `TimeoutsHolder` constructor.** The task holds the
holder and can run the
moment it is armed, and an event loop does not round a short deadline up
to a tick, so the
expiry could reach a holder whose fields were not yet frozen, a future
that had not been
handed the holder, and on the pooled path a future with no channel
attached - which aborted
with `null` and left the pooled socket open. The caller now publishes
the holder, attaches
the channel, and calls `start()` last.
Also from the review: `arm` re-checks `cancelled` after recording its
handle, so an exchange
that finishes mid-arming cannot leave behind an entry nobody will
cancel; `cancelArmed`
catches the `RejectedExecutionException` that Netty's off-loop
cancellation path can raise on
a closing client, which had never escaped `ListenableFuture#cancel`
before; the cancellation
handle is two typed fields rather than an `Object` and `instanceof`;
`requestTimeoutArmed` is
gone in favour of a null test on the task; the rationale lives on
`isUseEventLoopTimeouts()` alone; and the new option sits in the `//
timeouts` group
everywhere rather than splitting the two `failedIpCooldown` entries.
## API compatibility
No `revapi` entries. `implements Runnable` and `run()` live on the two
subclasses rather than
on `TimeoutTimerTask`, which leaves that class's surface unchanged: both
subclasses already
declared `run(Timeout)` without a throws clause, so inside a subclass
`run()` calls its own
override and has nothing to catch. No dead handler, and nothing
narrowed.
The knock-on is that only the concrete classes are both a `TimerTask`
and a `Runnable`, so
`TimeoutsHolder#arm` takes that intersection as a type parameter.
Everything else is
additive: the existing `TimeoutsHolder` constructor is kept and
delegates, and nothing is
removed.
`start()` is called from `NettyResponseFuture#setTimeoutsHolder` rather
than by the sender.
`TimeoutsHolder` has a public constructor in an exported package, and
splitting the arming
out of it would otherwise leave an outside caller free to install a
holder and get an
exchange with no request timeout at all.
## Tests
Four cases in `EventLoopTimeoutTest`, asserting where an expiry is
delivered from rather than
what it does. They hand the config their own `Timer` and
`EventLoopGroup` so the assertions
are against those objects and not against thread names, which a pool
name containing `timer`
or a configured thread factory would have broken with no bug present:
- the timer default, by identity against the timer's own thread;
- a connecting exchange, on the loop of the channel
`onTcpConnectSuccess` reported;
- an exchange on a pooled channel, on the loop of the channel
`onConnectionPooled` reported,
which is also what says it reused the connection rather than opening one
of its own;
- a read timeout under the new mode, which is armed after the request is
written and so
exercises a different arming path.
The group has eight loops. With two, a timeout armed on the wrong loop
is on the right one
half the time, and these assertions would have passed about half the
runs against the bug they
exist to catch; at eight, reverting `timeoutExecutor` to `next()` fails
the pooled case.
The connecting case, and the pooled case's first request, get a one
second budget on purpose.
A deadline reached before connecting would be delivered from the timer
quite correctly, there
being no channel to deliver it from, and would prove nothing either way;
and the pooled case's
first request is the cold one - class loading, the connect, the server's
first response - and
is meant to succeed.
One gap, called out rather than papered over: the
`RejectedExecutionException` fallback in
`arm` has no test. Reaching it needs a loop that answers
`isShuttingDown()` with `false` and
then rejects the schedule, and the executor comes from the channel, so
there is no way in
through the config. A test double for `EventExecutor` would do it if
that is acceptable.
## Verification
`mvnw clean verify` - BUILD SUCCESS, 1468 tests, 0 failures, 0 errors,
21 skipped. Error
Prone, NullAway clean; `revapi` clean with the one scoped entry above.
Caveat on the testing gate: `AGENTS.md` requires the build to run on JDK
11 and no JDK 11 is
installed 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](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>1 parent 793aae9 commit c1af3a4
12 files changed
Lines changed: 552 additions & 29 deletions
File tree
- client/src
- main
- java/org/asynchttpclient
- config
- netty
- channel
- request
- timeout
- resources/org/asynchttpclient/config
- test/java/org/asynchttpclient
Lines changed: 28 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
114 | 114 | | |
115 | 115 | | |
116 | 116 | | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
117 | 145 | | |
118 | 146 | | |
119 | 147 | | |
| |||
Lines changed: 23 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
97 | 97 | | |
98 | 98 | | |
99 | 99 | | |
| 100 | + | |
100 | 101 | | |
101 | 102 | | |
102 | 103 | | |
| |||
157 | 158 | | |
158 | 159 | | |
159 | 160 | | |
| 161 | + | |
160 | 162 | | |
161 | 163 | | |
162 | 164 | | |
| |||
258 | 260 | | |
259 | 261 | | |
260 | 262 | | |
| 263 | + | |
261 | 264 | | |
262 | 265 | | |
263 | 266 | | |
| |||
367 | 370 | | |
368 | 371 | | |
369 | 372 | | |
| 373 | + | |
370 | 374 | | |
371 | 375 | | |
372 | 376 | | |
| |||
585 | 589 | | |
586 | 590 | | |
587 | 591 | | |
| 592 | + | |
| 593 | + | |
| 594 | + | |
| 595 | + | |
| 596 | + | |
588 | 597 | | |
589 | 598 | | |
590 | 599 | | |
| |||
958 | 967 | | |
959 | 968 | | |
960 | 969 | | |
| 970 | + | |
961 | 971 | | |
962 | 972 | | |
963 | 973 | | |
| |||
1064 | 1074 | | |
1065 | 1075 | | |
1066 | 1076 | | |
| 1077 | + | |
1067 | 1078 | | |
1068 | 1079 | | |
1069 | 1080 | | |
| |||
1355 | 1366 | | |
1356 | 1367 | | |
1357 | 1368 | | |
| 1369 | + | |
| 1370 | + | |
| 1371 | + | |
| 1372 | + | |
| 1373 | + | |
| 1374 | + | |
| 1375 | + | |
| 1376 | + | |
| 1377 | + | |
| 1378 | + | |
| 1379 | + | |
1358 | 1380 | | |
1359 | 1381 | | |
1360 | 1382 | | |
| |||
1764 | 1786 | | |
1765 | 1787 | | |
1766 | 1788 | | |
| 1789 | + | |
1767 | 1790 | | |
1768 | 1791 | | |
1769 | 1792 | | |
| |||
Lines changed: 5 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
41 | 41 | | |
42 | 42 | | |
43 | 43 | | |
| 44 | + | |
44 | 45 | | |
45 | 46 | | |
46 | 47 | | |
| |||
154 | 155 | | |
155 | 156 | | |
156 | 157 | | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
157 | 162 | | |
158 | 163 | | |
159 | 164 | | |
| |||
Lines changed: 5 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
466 | 466 | | |
467 | 467 | | |
468 | 468 | | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
469 | 474 | | |
470 | 475 | | |
471 | 476 | | |
| |||
Lines changed: 5 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
124 | 124 | | |
125 | 125 | | |
126 | 126 | | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
127 | 132 | | |
128 | 133 | | |
129 | 134 | | |
| |||
Lines changed: 33 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
82 | 82 | | |
83 | 83 | | |
84 | 84 | | |
| 85 | + | |
85 | 86 | | |
86 | 87 | | |
87 | 88 | | |
| |||
401 | 402 | | |
402 | 403 | | |
403 | 404 | | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
404 | 410 | | |
405 | 411 | | |
406 | 412 | | |
407 | | - | |
| 413 | + | |
408 | 414 | | |
409 | 415 | | |
410 | | - | |
411 | | - | |
412 | | - | |
413 | 416 | | |
414 | 417 | | |
415 | 418 | | |
| |||
1080 | 1083 | | |
1081 | 1084 | | |
1082 | 1085 | | |
| 1086 | + | |
| 1087 | + | |
| 1088 | + | |
| 1089 | + | |
| 1090 | + | |
| 1091 | + | |
| 1092 | + | |
| 1093 | + | |
| 1094 | + | |
| 1095 | + | |
| 1096 | + | |
| 1097 | + | |
1083 | 1098 | | |
1084 | | - | |
1085 | | - | |
| 1099 | + | |
| 1100 | + | |
| 1101 | + | |
| 1102 | + | |
1086 | 1103 | | |
1087 | 1104 | | |
1088 | 1105 | | |
| 1106 | + | |
| 1107 | + | |
| 1108 | + | |
| 1109 | + | |
| 1110 | + | |
| 1111 | + | |
| 1112 | + | |
| 1113 | + | |
| 1114 | + | |
| 1115 | + | |
1089 | 1116 | | |
1090 | 1117 | | |
1091 | 1118 | | |
| |||
Lines changed: 10 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
25 | | - | |
| 25 | + | |
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
| |||
31 | 31 | | |
32 | 32 | | |
33 | 33 | | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
34 | 43 | | |
35 | 44 | | |
36 | 45 | | |
| |||
Lines changed: 10 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
25 | | - | |
| 25 | + | |
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
| |||
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
37 | 46 | | |
38 | 47 | | |
39 | 48 | | |
| |||
Lines changed: 65 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
18 | 19 | | |
| 20 | + | |
19 | 21 | | |
20 | 22 | | |
| 23 | + | |
21 | 24 | | |
22 | 25 | | |
23 | 26 | | |
24 | 27 | | |
| 28 | + | |
25 | 29 | | |
26 | 30 | | |
27 | 31 | | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
28 | 39 | | |
29 | 40 | | |
30 | 41 | | |
| |||
33 | 44 | | |
34 | 45 | | |
35 | 46 | | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
36 | 52 | | |
37 | 53 | | |
38 | 54 | | |
39 | 55 | | |
40 | 56 | | |
41 | 57 | | |
42 | 58 | | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
43 | 108 | | |
44 | 109 | | |
45 | 110 | | |
| |||
0 commit comments