fix(netty): never let a queued suspend override a later resume by mkurz · Pull Request #2361 · AsyncHttpClient/async-http-client

mkurz · 2026-10-07T08:15:07Z

Summary

A ResponseBodyControl.suspend() queued from another thread no longer takes effect after a resume() that the event loop made in the meantime, which could leave a response suspended with nothing left to resume it.
A resume() is still always applied, so it is never lost to a suspend() that a callback makes after it was queued.
Document when control calls take effect. No API change.

Problem

NettyResponseBodyControl applies a call made on the event loop before it returns, and queues a call made on any other thread to the event loop. Queued calls take effect in the order they were queued, without regard to calls the event loop made inline in the meantime.

The stall:

A consumer thread calls suspend(), which is queued.
Before it runs, a body callback on the event loop calls resume(), which takes effect immediately.
The queued suspend() then runs and suspends reads again.

Nothing calls resume() after that, so the response stalls until the request timeout. The callback's resume() was the latest decision, but the older suspend() overrode it.

This pattern occurs in reactive consumers that suspend from whichever thread finds the buffer full. Play WS hit a variant of it, and avoids it today by suspending only on the event loop.

Change

suspend() records a suspend request, and resume() clears it. On the event loop, suspend() suspends at once. From another thread, it queues the suspension.
A queued suspension is skipped if, when it would take effect, the most recent call to suspend() or resume() was a resume.
resume() is always applied, and calls made on the event loop take effect before they return, as before.

This is not "the latest call wins". Only suspensions can be skipped. Skipping a resume instead would reintroduce a lost-wakeup stall: a consumer takes the last buffered part and calls resume(), a callback on an older view calls suspend() before that resume is applied, and the resume must still win. A test pins this case.

Calls made concurrently on different threads are not ordered beyond these rules. For example, with suspend, resume and suspend queued from other threads, the first suspension still takes effect briefly, because the most recent call is a suspend when it runs; the queued resume and the second suspension then follow.

The ResponseBodyControl Javadoc now states when calls take effect:

a call on the event loop takes effect before it returns;
calls from other threads take effect later on the loop, possibly after further callbacks;
a resume() always takes effect;
a suspend() from another thread is skipped if the most recent call was a resume;
concurrent calls on different threads are not ordered otherwise.

Compatibility

No public API change. Behavior changes only for a suspend() from another thread whose queued suspension is overtaken by a resume(): it no longer takes effect. All other call orders behave as before.

AI disclosure

Claude Code on behalf of Matthias Kurz. The commits include Co-Authored-By per AGENTS.md.

Test plan

New tests, for HTTP/1.1, for HTTP/2, and as unit tests in NettyResponseBodyControlTest (with Netty auto-read on and off):

an earlier suspend() from another thread does not undo a later resume();
a resume() from another thread is not lost to a later suspend().

NettyResponseBodyControlTest also covers:

sequences of calls queued from another thread: suspend, resume, suspend ends suspended, and resume, suspend, resume ends resumed, with the exact number of suspensions started and ended;
a suspend() after a skipped one still takes effect;
a suspend() on the event loop takes effect even if a resume() from another thread clears its request between recording and acting on it. The channel's event loop is a proxy whose inEventLoop() runs that resume at exactly that moment.

The unit tests also check that the suspension start/end hooks stay balanced and that exactly one read is requested per resume. Calls from another thread go through a single-thread executor owned by each test, not the common pool.

Broken variants, each run against the 31 control tests:

A queued suspension always applied, as on main: 7 failures and 1 error (HTTP/2, by timeout). These are the tests that an earlier suspend() must not undo a later resume(), the sequence test, and the test of a suspension after a skipped one.
The latest call always wins, so a queued resume() is dropped for a later suspend(): 5 failures and 1 error (HTTP/2, by timeout). These are the tests that a resume() must not be lost, and the sequence test.
A skipped suspension that still runs the suspension start hook: 4 failures, all in NettyResponseBodyControlTest, through the counts of started suspensions. The integration tests don't catch this variant.
A suspend() on the event loop that still goes through the queued-suspend check, as in the first revision: 1 failure, the race test.

Runs:

ResponseBodyControlTest, Http2ResponseBodyControlTest and NettyResponseBodyControlTest on JDK 11: 31 tests passed.
./mvnw -B -ntp -Dgpg.skip=true clean verify on JDK 11: 1,804 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used.
The same 31 tests, compiled on JDK 11 and run on JDK 17, 21 and 25 (Maven Surefire's -Djvm): 0 failures on each.
All four open AHC changes merged together (raw Cookie headers on retries, ResponseBodyControl.execute, suspend/resume order, demand-bounded decompression): they merge without conflicts, together and in every pair, and ./mvnw -B -ntp -Dgpg.skip=true clean install on JDK 11 passes with 1,861 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. Rerun with the review fixes of this pull request and fix(netty): stop inflating a suspended HTTP/1.1 response #2362.

A ResponseBodyControl call made off the event loop is queued and takes effect later on the loop, in the order the calls were queued. A suspend() queued from another thread could therefore take effect after a resume() that the event loop made in the meantime, for example from a body callback, and leave reads suspended with nothing left to resume them. Record a suspend request when suspend() is called and clear it when resume() is called. A queued suspend() is skipped if, when it would take effect, the most recent call was a resume(). A resume() is always applied, so it is never lost to a suspend() that a callback made after it was queued. Calls made on the event loop take effect before they return, as before. Calls made concurrently on different threads are not ordered beyond that. Document when control calls take effect. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

hyperxpro · 2026-10-21T20:46:01Z

    public void suspend() {
-        execute(this::suspend0);
+        suspendRequested = true;
+        execute(this::applySuspend);


On the event loop this still goes through applySuspend. A resume from another thread can clear the flag between the write above and that check and then this suspend does nothing. That is not what the javadoc promises for calls made on the loop. Call suspend0 directly when inEventLoop and only queue applySuspend from other threads. A queued resume still wins afterwards so nothing else changes.

Agreed, fixed in 1479ab2. On the event loop, suspend() now calls suspend0() directly, and only a call from another thread is queued through applySuspend. A resume queued from another thread still runs afterwards and wins.

NettyResponseBodyControlTest.suspendOnTheEventLoopTakesEffectWhenAResumeClearsTheRequestFirst hits the window deterministically, without a hook in the production code. Its channel's event loop is a proxy whose inEventLoop(), asked from the event loop thread, first runs a resume() on another thread. That resume clears the request between suspend() recording it and acting on it. The test checks that the suspension took effect at once and that the queued resume ends it afterwards. With the previous suspend(), it fails.

hyperxpro · 2026-10-21T20:46:01Z

    private final boolean previousAutoRead;

    private final AtomicBoolean active = new AtomicBoolean(true);
+    // Set by suspend() and cleared by resume() before they are applied, so that a suspend() queued before a later


This explains the same thing as the comment in applySuspend. One is enough, I would keep the one next to the check. Also it is not strictly the most recent call that wins when both callers are off the loop, it is whichever task is queued last. Something like: a queued suspend is skipped if a resume was called after it, a resume is never skipped.

Done in 1479ab2. Only the comment next to the check remains, now: "A queued suspend() is skipped if resume() was called after the latest suspend(), so it cannot undo a newer resume(). A resume() is never skipped, so it is not lost to a suspend() that a callback decided on an older view."

hyperxpro · 2026-10-21T20:46:01Z

+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    public void queuedCallsFromAnotherThreadEndInTheStateOfTheLatestCall(boolean autoRead) throws Exception {


This one is green on main too, and it is also green if resume0 honours the flag (latest call wins). So it catches neither mistake. Assert the exact counts, 3 started after the first half and 3 ended after the second, and it fails on both. Or just drop it.

Thanks, I kept it with exact counts (7e26abc): 3 started and 2 ended after the first sequence, 3 and 3 after the second. It now fails on main (4 started, from the queued suspend that should be skipped) and with "latest call wins" (2 started).

hyperxpro · 2026-10-21T20:46:01Z

+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    public void resumeFromAnotherThreadIsNotLostToALaterSuspend(boolean autoRead) throws Exception {


This and the two integration versions pass on main as well. They are the only tests that fail if someone makes resume0 honour the flag, so worth a one line comment saying so. Otherwise the next person who sees them green on main removes them.

Added a two-line comment to it and to the two integration versions (7e26abc): they also pass when no queued suspend is skipped, and fail if a queued resume is skipped for a later suspend.

hyperxpro · 2026-10-21T20:46:01Z

+
+    // The calling event loop stays busy until the call returns, so the control has to queue the call behind it.
+    private static void callFromAnotherThread(Runnable call) throws Exception {
+        CompletableFuture.runAsync(call).get(5, SECONDS);


This blocks the event loop on the common pool. With parallel forks on a small box the pool can be busy and this times out for no reason. Better use a single thread executor owned by the test. Same helper in the other two test classes.

Agreed. All three test classes now call from another thread through a single-thread executor that each test owns and shuts down afterwards (7e26abc).

hyperxpro · 2026-10-21T20:46:02Z

    }

+    @Test
+    public void earlierSuspendFromAnotherThreadDoesNotUndoLaterResume() throws Exception {


The twin test below also fetches /large-sibling and checks connectionCount. Do the same here. A skipped suspend that still ran the started hook would leave the flow controller in refill mode and this test would not notice.

Done (7e26abc). The test now fetches /large-sibling afterwards and checks connectionCount, as its twin does.

To be precise about what it catches: I tried a variant where a skipped suspend still runs the start hook, and this test still passes. The sibling stream completes over the same connection anyway, so the flow controller left in refill mode doesn't show up there. NettyResponseBodyControlTest does catch that variant, through the exact counts of started and ended suspensions (comment 3).

Motivation: suspend() recorded the suspend request and then ran applySuspend, inline when called on the event loop. A resume() from another thread could clear the request in between, and the suspend() on the event loop then did nothing, although a call made on the event loop is documented to take effect before it returns. Modification: On the event loop, suspend() now suspends directly. Only a suspend() from another thread is queued, and only a queued one can be skipped. A resume() queued from another thread still takes effect afterwards. Keep a single comment on the skip rule, next to the check. Result: A suspend() on the event loop always takes effect before it returns. A test runs a resume() from another thread at the moment suspend() asks whether it is on the event loop; it fails without this change. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- Assert the exact number of suspensions started and ended for the sequences of queued calls, so that test fails both on main and if the latest call always won. - Explain why the tests that a resume is not lost to a later suspend pass without skipping too: they fail if a resume is skipped. - Call from another thread through a single-thread executor owned by the test instead of the common pool, which can be busy on a small machine while the event loop waits for it. - After the HTTP/2 test of an earlier suspend, check that a sibling stream on the same connection still gets its whole body, as the twin test does. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mkurz · 2026-10-22T10:41:54Z

Thanks for the review. I've pushed two commits on top: the fix for a suspend() on the event loop, and the test changes. I also updated the description. Replies are inline.

mkurz mentioned this pull request Oct 7, 2026

Upgrade to AHC 3.0.15 and decide streamed-body resumes through ResponseBodyControl.execute playframework/play-ws#1487

Draft

18 of 22 tasks

hyperxpro requested changes Oct 21, 2026

View reviewed changes

mkurz and others added 2 commits October 22, 2026 01:27

mkurz mentioned this pull request Oct 22, 2026

fix(netty): stop inflating a suspended HTTP/1.1 response #2362

Open

17 tasks done

mkurz requested a review from hyperxpro October 22, 2026 10:48

Navigation Menu

Repository navigation

fix(netty): never let a queued suspend override a later resume - #2361

fix(netty): never let a queued suspend override a later resume#2361
mkurz wants to merge 3 commits into
AsyncHttpClient:mainfrom
mkurz:fix/suspend-resume-order

mkurz commented Oct 7, 2026 •

edited

Loading

Uh oh!

hyperxpro Oct 21, 2026

Uh oh!

mkurz Oct 22, 2026

Uh oh!

hyperxpro Oct 21, 2026

Uh oh!

mkurz Oct 22, 2026

Uh oh!

hyperxpro Oct 21, 2026

Uh oh!

mkurz Oct 22, 2026

Uh oh!

hyperxpro Oct 21, 2026

Uh oh!

mkurz Oct 22, 2026

Uh oh!

hyperxpro Oct 21, 2026

Uh oh!

mkurz Oct 22, 2026

Uh oh!

hyperxpro Oct 21, 2026

Uh oh!

mkurz Oct 22, 2026

Uh oh!

mkurz commented Oct 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mkurz commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Change

Compatibility

Related

AI disclosure

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurz commented Oct 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mkurz commented Oct 7, 2026 •

edited

Loading