Skip to content

Use field updaters for timeout guards - #2263

Closed
pavel-ptashyts wants to merge 1 commit into
AsyncHttpClient:mainfrom
maygemdev:perf/timeout-field-updaters
Closed

pavel-ptashyts wants to merge 1 commit into
AsyncHttpClient:mainfrom
maygemdev:perf/timeout-field-updaters

Conversation

@pavel-ptashyts

@pavel-ptashyts pavel-ptashyts commented Jul 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • replace per-instance timeout AtomicBoolean guards with volatile int fields managed by static AtomicIntegerFieldUpdaters
  • preserve existing run-once, reschedule, and cleanup semantics
  • add unit coverage for idempotent timeout task cleanup

Verification

  • ./mvnw -pl client -DskipTests compile
  • ./mvnw -pl client -Dtest=TimeoutTimerTaskTest,PerRequestTimeoutTest,NettyConnectListenerTest test

Attribution

Codex on behalf of Pavel Ptashyts

Replace per-instance AtomicBoolean guards in TimeoutsHolder and
TimeoutTimerTask with volatile int fields managed by static
AtomicIntegerFieldUpdaters. This preserves run-once and cleanup
semantics while avoiding an extra guard object allocation per timeout
holder and task.

Add unit coverage for idempotent TimeoutTimerTask cleanup.

Codex on behalf of Pavel Ptashyts

Co-Authored-By: Codex <codex@openai.com>
@pavel-ptashyts
pavel-ptashyts force-pushed the perf/timeout-field-updaters branch from 19a8b49 to 863afec Compare July 20, 2026 08:50
@pavel-ptashyts

Copy link
Copy Markdown
Contributor Author

Closing this for the 3.x line because Revapi correctly reports the removal of the protected TimeoutTimerTask.done field as an incompatible API change. Retaining the AtomicBoolean field would retain the allocation this change is intended to remove, while adding a Revapi exception would silently break external subclasses. This optimization can be reconsidered for a major release; the branch is retained as a reference.

@pavel-ptashyts
pavel-ptashyts deleted the perf/timeout-field-updaters branch July 20, 2026 14:35
hyperxpro pushed a commit that referenced this pull request Nov 9, 2026
## Summary

Every attempt builds a `TimeoutsHolder`, arms its request timeout and,
once the request is written, its read timeout, then cancels both when
the exchange ends. When the read timeout is shorter than the request
timeout, that is two timer entries per attempt, and almost every
exchange cancels both unfired. On the client's `HashedWheelTimer` each
entry is an increment of a client-wide `AtomicLong` and an offer to a
client-wide MPSC queue, each cancel an offer to a second one, and the
timer's single thread drains them all. With `useEventLoopTimeouts` each
is a scheduled task added to and removed from the loop.

Changes:

1. **`RequestTimeoutsArmBenchmark`** measures one attempt's timeout
lifecycle on a real `HashedWheelTimer`, with the read timeout shorter
and not shorter than the request timeout.
**`RequestTimeoutsFireBenchmark`** measures an exchange that outlives
its read window: the entry that checks the read timeout fires one or
five times, finding a recent read and arming again each time.
2. **Private timeout guards in field updaters.** The two
`AtomicBoolean`s of `TimeoutsHolder` and the suspension-warning flag of
`ReadTimeoutTimerTask` become volatile ints behind static
`AtomicIntegerFieldUpdater`s. `TimeoutTimerTask.done` stays an
`AtomicBoolean`: it is a protected field of a public class, and
replacing it is the API break that closed #2263.
3. **One timer entry per attempt.** When the read timeout is the shorter
one, `start()` arms the request timeout's entry for when a read timeout
would be due. Once the read timeout starts it rides on that entry
instead of taking its own. On firing, the entry fails the exchange with
a read timeout when one is due, and otherwise re-arms for whichever
deadline comes next: what is left of the read window, capped at a whole
window and by the request deadline.
4. **Fixes from the review rounds below**, each with a test that fails
without it:
- the request deadline that the shared entry re-arms and checks against
is kept on the monotonic clock (an absolute deadline straight off the
exchange's start), so a wall clock step cannot move it;
- each arming takes a number from a per-task counter, and an entry is
recorded only if no later arming has begun, so an arming that loses the
race against its own firing cannot hide the newer entry from `cancel()`;
the handle is one field, taken atomically on cancel;
- if the timer rejects the re-armed entry (a user timer that is full or
stopped), the exchange is failed instead of being left with no deadline,
but only while this holder is still the future's and the future is not
done;
- the monotonic clock is read only where the deadline is needed (an
entry armed early, or `useEventLoopTimeouts`), so the default attempt
costs no more than on `main`;
- an entry that fires early reads the monotonic clock once, both to
check the request deadline and to time the next entry; it skips the
read-state CAS once the read timeout is covered, and writes the early
flag only when the flag changes. Without these, five fires per exchange
ran slower than `main`'s separate read entry.

## Behaviour notes

- Same timeouts, same messages, same exceptions.
- A read timeout that has not started when the shared entry fires (a
slow connect or upload) goes back to an entry of its own, as before.
That costs one fire and re-arm that `main` does not have.
- An exchange that outlives its read window re-arms the shared entry for
what is left of the window, capped at a whole window. #2377 re-arms
`main`'s separate read entry the same way. Each fire arms one entry, as
each fire of the read entry does on `main`, and the request timeout's
own entry is gone.
- An entry that fires past the request deadline (a wheel tick can land
after it) delivers the request timeout.
- A suspended response body still holds off the read timeout for a whole
window, and the indefinite-suspension warning is unchanged.
- Absolute deadlines, `rehomeOn`, `useEventLoopTimeouts` and cancel
without pinning the future are kept; their existing tests pass
unchanged.
- No public API change: Revapi passes. New members are private or
package-private; `TimeoutsHolder.cancel()` keeps its signature.

## Relation to #2377

#2377 makes the separate read timeout entry re-arm for what is left of
its window rather than a whole one. It has since merged into `main`.
This branch still merges into `main` cleanly (`git merge-tree` against
f5ad541), and no merge of `main` was pushed here. Before #2377 merged,
both PRs' timeout tests passed on the merged tree. The shared entry here
re-arms for the remaining window the same way, capped at a whole window
as #2377 caps it.

## Benchmark

`RequestTimeoutsArmBenchmark.armAndCancel`, request timeout 60 s. JMH
1.37, JDK 17.0.2, Windows, `-prof gc`, 3 forks, 3 x 2 s warmup, 5 x 2 s
measurement, `main` against 12291df. The later commits change the
firing path; on the arming path they only skip a write of a flag that
did not change.

Read timeout 10 s (two entries on `main`, one here):

| Threads | `main` (ops/us) | This PR (ops/us) | Bytes/op |
| ---: | ---: | ---: | ---: |
| 1 | 2.76 +- 0.02 | 3.57 +- 0.03 | 391 -> 223 |
| 8 | 5.35 +- 1.13 | 10.71 +- 3.11 | 392 -> 225 |
| 32 | 1.74 +- 0.52 | 3.36 +- 0.96 | 395 -> 226 |

Read timeout 60 s, the default (one entry either way):

| Threads | `main` (ops/us) | This PR (ops/us) | Bytes/op |
| ---: | ---: | ---: | ---: |
| 1 | 3.99 +- 0.03 | 3.97 +- 0.02 | 247 -> 223 |
| 8 | 7.43 +- 4.59 | 11.08 +- 2.89 | 249 -> 225 |
| 32 | 3.91 +- 1.05 | 4.32 +- 1.07 | 249 -> 226 |

`RequestTimeoutsFireBenchmark.armFireAndCancel`, read timeout 10 s,
request timeout 60 s, same settings, `main` against 44b7bc3. The
benchmark fires the entry from its own thread by taking it out of the
wheel (a cancel) and running its task. Both builds pay that cancel once
per fire. At one thread, `main` and this branch ran alternately, twice
each:

| Threads | Fires | `main` (ops/us) | This PR (ops/us) | Bytes/op |
| ---: | ---: | ---: | ---: | ---: |
| 1 | 1 | 1.67 +- 0.32, 1.71 +- 0.32 | 1.99 +- 0.33, 1.88 +- 0.34 | 455
-> 287 |
| 1 | 5 | 0.76 +- 0.20, 0.61 +- 0.12 | 0.99 +- 0.10, 0.89 +- 0.14 | 709
-> 541 |
| 8 | 1 | 1.71 +- 1.20 | 5.07 +- 0.82 | 459 -> 290 |
| 8 | 5 | 1.27 +- 0.17 | 1.31 +- 0.45 | 716 -> 550 |
| 32 | 1 | 1.23 +- 0.39 | 1.44 +- 0.53 | 460 -> 292 |
| 32 | 5 | 0.51 +- 0.20 | 0.67 +- 0.27 | 722 -> 553 |

The host was shared with other builds while these ran, so the
multi-threaded rows have wide error bars; allocation is the reliable
figure there.

## Tests

New `RequestTimeoutTimerTaskTest` (26 tests) drives a holder with fake
wall and monotonic clocks and a recording timer and event loop. It
covers one entry serving both timeouts, re-arming for the rest of the
read window, delivery of read and request timeouts from the shared
entry, a read timeout that starts late or while the entry re-arms,
absolute deadlines, wall clock steps both ways, suspended bodies,
`rehomeOn` before the read timeout starts, cancellation after an early
fire on the timer and on a loop, an arming that loses the race to its
own firing, a re-arm the timer rejects (with a live, a replaced and a
detached holder), a request timeout of `Long.MAX_VALUE` ms whose
deadline wraps on the monotonic clock, a completed exchange fired inside
its read window, the default attempt not reading the monotonic clock,
and an early fire reading it once.

`./mvnw clean verify -Dgpg.skip=true` passes on JDK 17 (1965 tests; no
JDK 11 and no gpg on the host); CI covers JDK 11.

## Maintainer review

Eight comments from hyperxpro, written against c8df6c3. At 12291df,
five were already fixed: the deadline on the monotonic clock with
subtraction, the one-window cap, the `Long.MAX_VALUE` overflow, the
atomic take of the handle in `cancelArmed()`, and the abort on a
rejected re-arm. The `done.set(false)` ordering test already existed
(`aReadTimeoutStartingWhileTheEntryReArmsGetsItsOwnEntry`). This push
adds the rest: the overflow test, the completed-exchange test fired
inside the window, `RequestTimeoutsFireBenchmark`, and the per-fire
trims it led to. Both new review tests fail when the code they guard is
reverted, and so does the existing ordering test when `done.set(false)`
moves below the re-arm.

## Second-agent review

Both reviewers ran from fresh CLI sessions, read-only, on `git diff
upstream/main...HEAD`, with the intent and constraints in the prompt.

| Reviewer | Agent and model | Effort | Commit reviewed | Result |
| --- | --- | --- | --- | --- |
| Guided (against `AGENTS.md`) | Codex `gpt-6-astra` | medium |
12291df | No actionable findings |
| Independent (no checklist) | Claude Code `claude-opus-5-5` | medium |
12291df | No actionable findings; two optional notes, no change made |

Seven rounds in all (c8df6c3, f2cddcc, 3daa3b7, cbb6a78,
fe9fc58, e678f09, 12291df). Every actionable finding of the
earlier rounds was fixed in the commits listed under change 4, except
these, which were not changed:

- **`startReadTimeout(task)` still compares the read timeout against the
request deadline on the wall clock.** Inherited from `main`. #2377, now
merged, rewrote the same line. Changing it here would need `main` merged
into this branch, so it is left for a follow-up after this PR.
- **Check-then-act window when a rejected re-arm aborts the exchange.**
After the ownership check passes, a redirect or completion can still
replace the holder or pool the channel before `abort` runs. Every
timeout delivery on `main` has the same window (`expire()` checks and
then aborts with the future's current channel from the timer thread);
closing it means serializing timeout delivery with attempt replacement
on the channel's event loop, a redesign outside this PR. The path is
reached only when a user-supplied timer rejects an entry.
- **`start()` sets the pending read state with a plain write** (round 6,
independent reviewer, low). A read timeout started on a new holder
before `start()` runs can leave both a separate read entry and the early
entry; `main` double-arms the read timeout in the same race, and the two
entries run on the same scheduler, so the overlap that would let one
drop the other does not arise outside a `rehomeOn`.
- **`rehomeOn` javadoc** still says it is a no-op once the request
timeout has fired; an early firing of the shared entry is not that, and
the next re-arm reads the new executor anyway. Wording only.

## Follow-up idea, not in this PR

A per-connection lazy deadline for pooled HTTP/1.1 connections, in the
style of `IdleStateHandler`: one entry per connection, re-armed only
when it fires early or a new deadline is earlier, so a keep-alive
request touches neither the timer nor a `TimeoutsHolder`. A throwaway
prototype put the per-request cost near zero, but it is a new
synchronization protocol at the exchange handover on a pooled channel,
so it should wait for an end-to-end keep-alive benchmark that shows how
much of a whole request this cost is.

Claude Code on behalf of Pavel Ptashyts

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant