FlowControlHandler: respect auto-read when toggled while dequeueing by schiemon · Pull Request #16949 · netty/netty

schiemon · 2026-06-13T15:00:07Z

Motivation

FlowControlHandler does not respect changes to the channel's auto-read setting while flushing the queue. As such, a downstream handler that disables auto-read from within channelRead() cannot stop FlowControlHandler from flushing the rest of the queue.

Modification

Merge dequeueOne() and dequeueAll() into a single dequeue() loop that re-checks config.isAutoRead() and unsatisfiedReads before every message.
Add tests for auto-read toggled on and off from channelRead and re-entrant reads satisfied from the queue, plus previously missing coverage for handler removal, channelInactive release, and releaseMessages = false.

Result

A downstream handler that disables auto-read from channelRead() now stops delivery of the rest of the queue.

Fixes #16945

schiemon · 2026-06-14T18:26:26Z

Need to have another look at this tomorrow before undrafting

normanmaurer · 2026-06-15T07:24:47Z

@schiemon just ping us when it's ready.

schiemon · 2026-06-15T12:10:41Z

@chrisvest @normanmaurer ready for review 👍

normanmaurer · 2026-06-15T14:12:46Z

+        if (!dequeue(ctx) || config.isAutoRead()) {
+            assert unsatisfiedReads > 0 || config.isAutoRead();


You need to store autoRead in a variable as otherwise it might be modified by another thread in between the if and the assert.

Suggested change

if (!dequeue(ctx) || config.isAutoRead()) {

assert unsatisfiedReads > 0 || config.isAutoRead();

boolean autoRead = config.isAutoRead();

if (!dequeue(ctx) || autoRead {

assert unsatisfiedReads > 0 || autoRead;

Good point but we need to be careful here and capture the auto-read setting after dequeueing as it can change in downstream channelRead handlers that are invoked by dequeue

Motivation: - FlowControlHandler does not respect changes to the channel's auto-read setting while flushing the queue. As such, a downstream handler that disables auto-read from within channelRead() cannot stop FlowControlHandler from flushing the rest of the queue. Modification: - Merge dequeueOne() and dequeueAll() into a single dequeue() loop that re-checks config.isAutoRead() and unsatisfiedReads before every message. - Add tests for auto-read toggled on and off from channelRead and re-entrant reads satisfied from the queue, plus previously missing coverage for handler removal, channelInactive release, and releaseMessages = false. Result: - A downstream handler that disables auto-read from channelRead() now stops delivery of the rest of the queue. Fixes netty#16945

normanmaurer · 2026-06-19T06:47:44Z

@lhotari can you check ?

lhotari · 2026-06-22T10:10:59Z

@lhotari can you check ?

@normanmaurer LGTM. This suites Apache Pulsar use cases. Thanks for fixing, @schiemon.

normanmaurer · 2026-06-22T14:49:09Z

@chrisvest @yawkat PTAL

chrisvest

I was speculating if there was a reentrancy bug, and it took me a day to chase down and verify that no there isn't.

Looks good.

netty-project-bot · 2026-06-24T08:50:45Z

Could not create auto-port PR.
Got conflicts when cherry-picking onto 5.0.

netty-project-bot · 2026-06-24T08:50:53Z

Auto-port PR for 4.1: #16983

normanmaurer · 2026-06-24T09:00:14Z

PR for 5.0 #16984

…le dequeueing (#16983) Auto-port of #16949 to 4.1 Cherry-picked commit: 8081e23 --- ## Motivation `FlowControlHandler` does not respect changes to the channel's auto-read setting while flushing the queue. As such, a downstream handler that disables auto-read from within `channelRead()` cannot stop `FlowControlHandler` from flushing the rest of the queue. ## Modification - Merge `dequeueOne()` and `dequeueAll()` into a single `dequeue()` loop that re-checks `config.isAutoRead()` and `unsatisfiedReads` before every message. - Add tests for auto-read toggled on and off from `channelRead` and re-entrant reads satisfied from the queue, plus previously missing coverage for handler removal, `channelInactive` release, and `releaseMessages = false`. ## Result A downstream handler that disables auto-read from `channelRead()` now stops delivery of the rest of the queue. Fixes #16945 Co-authored-by: Szymon Habrainski <56340221+schiemon@users.noreply.github.com> Co-authored-by: Norman Maurer <norman_maurer@apple.com>

…16949) (#16984) `FlowControlHandler` does not respect changes to the channel's auto-read setting while flushing the queue. As such, a downstream handler that disables auto-read from within `channelRead()` cannot stop `FlowControlHandler` from flushing the rest of the queue. - Merge `dequeueOne()` and `dequeueAll()` into a single `dequeue()` loop that re-checks `config.isAutoRead()` and `unsatisfiedReads` before every message. - Add tests for auto-read toggled on and off from `channelRead` and re-entrant reads satisfied from the queue, plus previously missing coverage for handler removal, `channelInactive` release, and `releaseMessages = false`. A downstream handler that disables auto-read from `channelRead()` now stops delivery of the rest of the queue. Fixes #16945 Co-authored-by: Norman Maurer <norman_maurer@apple.com> Co-authored-by: Szymon Habrainski <56340221+schiemon@users.noreply.github.com>

ursaj · 2026-07-03T01:43:09Z

        // Upstream closed the read cycle. Collapse every outstanding read() into a single downstream
        // channelReadComplete; spurious upstream completions with no pending read are dropped.
        if (config.isAutoRead() || unsatisfiedReads > 0) {
            unsatisfiedReads = 0;


why reset? wouldn't this force downstream(s) to manage unsatisfied reads manually?

if (config.isAutoRead()) { ctx.fireChannelReadComplete(); } else if (unsatisfiedReads > 0) { ctx.read(); }

tested our systems with and without this patch against the latest origin/4.2 state:

without patch (unsatisfiedReads = 0 + readComplete event) – ~7k tests failures

with patch (if (unsatisfiedReads > 0) ctx.read();) – all green

schiemon force-pushed the fix/flowcontrolhandler-autoread-16945 branch 2 times, most recently from 5a521c8 to 4ceff75 Compare June 13, 2026 16:33

schiemon force-pushed the fix/flowcontrolhandler-autoread-16945 branch 2 times, most recently from 8eda866 to 4a2aead Compare June 15, 2026 09:55

schiemon marked this pull request as ready for review June 15, 2026 12:08

normanmaurer requested changes Jun 15, 2026

View reviewed changes

schiemon force-pushed the fix/flowcontrolhandler-autoread-16945 branch 3 times, most recently from 3024057 to 32eb674 Compare June 16, 2026 09:49

schiemon force-pushed the fix/flowcontrolhandler-autoread-16945 branch from 32eb674 to a3e104b Compare June 16, 2026 09:51

Merge branch '4.2' into fix/flowcontrolhandler-autoread-16945

086059d

schiemon requested a review from normanmaurer June 16, 2026 10:16

Merge branch '4.2' into fix/flowcontrolhandler-autoread-16945

998b484

normanmaurer added this to the 4.2.16.Final milestone Jun 22, 2026

normanmaurer added needs-cherry-pick-4.1 This PR should be cherry-picked to 4.1 once merged. needs-cherry-pick-5.0 This PR should be cherry-picked to 5.0 once merged. labels Jun 22, 2026

normanmaurer requested review from chrisvest and yawkat June 22, 2026 14:48

chrisvest approved these changes Jun 23, 2026

View reviewed changes

normanmaurer merged commit 8081e23 into netty:4.2 Jun 24, 2026
38 of 39 checks passed

netty-project-bot mentioned this pull request Jun 24, 2026

Auto-port 4.1: FlowControlHandler: respect auto-read when toggled while dequeueing #16983

Merged

github-actions Bot removed the needs-cherry-pick-4.1 This PR should be cherry-picked to 4.1 once merged. label Jun 24, 2026

normanmaurer removed the needs-cherry-pick-5.0 This PR should be cherry-picked to 5.0 once merged. label Jun 24, 2026

chrisvest mentioned this pull request Jul 2, 2026

FlowControlHandler: Suppress duplicate channelReadComplete after draining queue (#15053) #16837

Merged

ursaj reviewed Jul 3, 2026

View reviewed changes

ursaj mentioned this pull request Jul 3, 2026

FlowControlHandler must comply with its fundamental goal (ensured message for each read request) - behaviour broken in 4.2.15.Final (#17050) #17046

Open

		if (!dequeue(ctx) \|\| config.isAutoRead()) {
		assert unsatisfiedReads > 0 \|\| config.isAutoRead();

-        if (!dequeue(ctx) || config.isAutoRead()) {
-            assert unsatisfiedReads > 0 || config.isAutoRead();
+        boolean autoRead = config.isAutoRead();
+        if (!dequeue(ctx) || autoRead {
+            assert unsatisfiedReads > 0 || autoRead;

Uh oh!

Uh oh!

Conversation

schiemon commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modification

Result

Uh oh!

schiemon commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

normanmaurer commented Jun 15, 2026

Uh oh!

schiemon commented Jun 15, 2026

Uh oh!

normanmaurer Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

schiemon Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Jun 19, 2026

Uh oh!

lhotari commented Jun 22, 2026

Uh oh!

normanmaurer commented Jun 22, 2026

Uh oh!

chrisvest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

netty-project-bot commented Jun 24, 2026

Uh oh!

netty-project-bot commented Jun 24, 2026

Uh oh!

normanmaurer commented Jun 24, 2026

Uh oh!

ursaj Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

ursaj Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

schiemon commented Jun 13, 2026 •

edited

Loading

schiemon commented Jun 14, 2026 •

edited

Loading

schiemon Jun 16, 2026 •

edited

Loading