Skip to content

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

Open
mkurz wants to merge 1 commit into
AsyncHttpClient:mainfrom
mkurz:fix/suspend-resume-order
Open

mkurz wants to merge 1 commit into
AsyncHttpClient:mainfrom
mkurz:fix/suspend-resume-order

Conversation

@mkurz

@mkurz mkurz commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

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:

  1. A consumer thread calls suspend(), which is queued.
  2. Before it runs, a body callback on the event loop calls resume(), which takes effect immediately.
  3. 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 before it runs or queues the actual suspension, and resume() clears that request.
  • 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.

Related

The separate pull request #2359, "feat: add ResponseBodyControl.execute to decide in sequence with body delivery", lets a consumer make its suspend/resume decision on the event loop in the first place. The two changes are independent and merge cleanly with each other.

AI disclosure

Claude Code on behalf of Matthias Kurz. The commit includes 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;
  • a suspend() after a skipped one still takes effect.

The unit tests also check that the suspension start/end hooks stay balanced and that exactly one read is requested per resume.

Broken variants, each run against the 30 control tests:

  • A queued suspension always applied, as on main: 5 failures and 1 error (HTTP/2, by timeout). These are the tests that an earlier suspend() must not undo a later resume(), 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(): 3 failures and 1 error (HTTP/2, by timeout). These are the tests that a resume() must not be lost.

The sequence test is not caught by either variant: it pins the documented outcome rather than a specific bug.

Runs:

  • ResponseBodyControlTest, Http2ResponseBodyControlTest and NettyResponseBodyControlTest on JDK 11: 30 tests passed.
  • ./mvnw -B -ntp -Dgpg.skip=true clean verify on JDK 11: 1,803 tests, 0 failures, 0 errors, 22 skipped; Revapi passed. No test-skipping flags were used.
  • The same 30 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,853 tests, 0 failures, 0 errors, 22 skipped; Revapi passed.

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>
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