Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FlushConsolidationHandler may suppress flushes by mistake #9931

Merged
merged 4 commits into from
Jan 9, 2020
Merged

FlushConsolidationHandler may suppress flushes by mistake #9931

merged 4 commits into from
Jan 9, 2020

Conversation

carryxyh
Copy link
Contributor

@carryxyh carryxyh commented Jan 9, 2020

Motivation:

When consolidatedWhenNoReadInProgress is true, channel.writeAndFlush (data) .addListener (f-> channel.writeAndFlush (data2)) Will cause data2 to never be flushed.

Because the flush operation will synchronously execute the channel.writeAndFlush (data2)) in the listener, and at this time, since the current execution thread is still an eventloop(executor.inEventLoop() was true), all handlers will be executed synchronously. At this time, since nextScheduledFlush is still not null, the flush operation of data2 will be ignored in FlushConsolidationHandler#scheduleFlush.

Modification:

  • reset nextScheduledFlush before ctx.flush
  • use ObjectUtil to polish code

Result:

Fixes #9923

@carryxyh carryxyh mentioned this pull request Jan 9, 2020
Closed
@netty-bot
Copy link

Can one of the admins verify this patch?

@normanmaurer
Copy link
Member

@carryxyh can you add a unit test as well ?

@carryxyh
Copy link
Contributor Author

carryxyh commented Jan 9, 2020

done. pls check..

normanmaurer
normanmaurer requested changes Jan 9, 2020
View reviewed changes
handler/src/test/java/io/netty/handler/flush/FlushConsolidationHandlerTest.java Outdated
assertEquals(1L, channel.readOutbound());
assertNull(channel.readOutbound());

channel.finish();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: assertFalse(channel.finish);

handler/src/test/java/io/netty/handler/flush/FlushConsolidationHandlerTest.java Outdated
*/
@Test
public void testResend() throws Exception {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@carryxyh please fix check style:

11:21:33 [INFO] --- maven-checkstyle-plugin:3.0.0:check (check-style) @ netty-handler ---
11:21:35 [INFO] Starting audit...
11:21:35 [ERROR] /code/handler/src/main/java/io/netty/handler/flush/FlushConsolidationHandler.java:99: Line is longer than 120 characters (found 122). [LineLength]
11:21:35 Audit done.

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@carryxyh thanks a lot!

@normanmaurer normanmaurer merged commit 2be3d88 into netty:4.1 Jan 9, 2020
@normanmaurer normanmaurer added this to the 4.1.45.Final milestone Jan 9, 2020
normanmaurer pushed a commit that referenced this pull request Jan 9, 2020
@carryxyh @normanmaurer
FlushConsolidationHandler may suppress flushes by mistake (#9931)
044b6b0 
Motivation:

When `consolidatedWhenNoReadInProgress` is true, `channel.writeAndFlush (data) .addListener (f-> channel.writeAndFlush (data2))` Will cause data2 to never be flushed.

Because the flush operation will synchronously execute the `channel.writeAndFlush (data2))` in the `listener`, and at this time, since the current execution thread is still an `eventloop`(`executor.inEventLoop()` was true), all handlers will be executed synchronously. At this time, since `nextScheduledFlush` is still not null, the `flush` operation of `data2` will be ignored in `FlushConsolidationHandler#scheduleFlush`.

Modification:

 - reset `nextScheduledFlush` before `ctx.flush`
 - use `ObjectUtil` to polish code

Result:

Fixes #9923
@carryxyh carryxyh deleted the fix-resend branch January 10, 2020 02:24
@normanmaurer normanmaurer changed the title Fix resend problem. FlushConsolidationHandler may suppress flushes by mistake Jan 13, 2020
@mtojek mtojek mentioned this pull request Jan 20, 2020
Open
ihanyong pushed a commit to ihanyong/netty that referenced this pull request Jul 31, 2020
@carryxyh @ihanyong
FlushConsolidationHandler may suppress flushes by mistake (netty#9931)
7e23306 
Motivation:

When `consolidatedWhenNoReadInProgress` is true, `channel.writeAndFlush (data) .addListener (f-> channel.writeAndFlush (data2))` Will cause data2 to never be flushed.

Because the flush operation will synchronously execute the `channel.writeAndFlush (data2))` in the `listener`, and at this time, since the current execution thread is still an `eventloop`(`executor.inEventLoop()` was true), all handlers will be executed synchronously. At this time, since `nextScheduledFlush` is still not null, the `flush` operation of `data2` will be ignored in `FlushConsolidationHandler#scheduleFlush`.

Modification:

 - reset `nextScheduledFlush` before `ctx.flush`
 - use `ObjectUtil` to polish code

Result:

Fixes netty#9923
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normanmaurer normanmaurer normanmaurer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.1.45.Final
Development

Successfully merging this pull request may close these issues.

FlushConsolidationHandler does not flush when write during flush
3 participants
@carryxyh @netty-bot @normanmaurer