Skip to content

Make sure we always flush window update frames in AbstractHttp2StreamChannel#10075

Merged
normanmaurer merged 1 commit into
netty:4.1from
bryce-anderson:banderson/issue-10072_flowconotrol
Mar 4, 2020
Merged

Make sure we always flush window update frames in AbstractHttp2StreamChannel#10075
normanmaurer merged 1 commit into
netty:4.1from
bryce-anderson:banderson/issue-10072_flowconotrol

Conversation

@bryce-anderson
Copy link
Copy Markdown
Contributor

Motivation:

Under certain read patters the AbstractHttp2StreamChannel can fail to
flush, resulting in flow window starvation.

Modifications:

  • Ensure we flush if we exit the doBeginRead() method.
  • Account for the Http2FrameCodec always synchronously finishing writes
    of window update frames.

Result:

Fixes #10072

Comment thread codec-http2/src/test/java/io/netty/handler/codec/http2/Http2TestUtil.java Outdated
@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

Copy link
Copy Markdown
Contributor Author

@bryce-anderson bryce-anderson Mar 2, 2020

Choose a reason for hiding this comment

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

This change isn't strictly necessary for this patch but I spent a fair amount of time trying to read through this for the nth time and decided to clean it up so the control flow (when the method returns etc, not H2 flow control) is more readable. If desired, we can move it to a different patch.

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer normanmaurer added this to the 4.1.47.Final milestone Mar 3, 2020
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

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

just a nit but generally speaking LGTM. Great catch!

Comment thread codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexTest.java Outdated
@bryce-anderson
Copy link
Copy Markdown
Contributor Author

@normanmaurer, presuming this gets accepted, what are the odds we could get a release relatively soon? I know this just missed the 4.1.46 release but this is a show stopper for us and looks to affect both clients and servers using the MultiplexHandler/MultiplexCodec API's.

@normanmaurer
Copy link
Copy Markdown
Member

I think a release could be done within a week or so :)

@bryce-anderson bryce-anderson force-pushed the banderson/issue-10072_flowconotrol branch from 9232b9a to eed3847 Compare March 3, 2020 18:25
Copy link
Copy Markdown
Contributor

@sskrobotov sskrobotov left a comment

Choose a reason for hiding this comment

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

Thank you for the patch! Coincidentally, just a few days back we've also discovered that we're affected by this issue.

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

…Channel

Motivation:

Under certain read patters the AbstractHttp2StreamChannel can fail to
flush, resulting in flow window starvation.

Modifications:

- Ensure we flush if we exit the `doBeginRead()` method.
- Account for the Http2FrameCodec always synchronously finishing writes
  of window update frames.

Result:

Fixes netty#10072
@bryce-anderson bryce-anderson force-pushed the banderson/issue-10072_flowconotrol branch from eed3847 to 1e63317 Compare March 3, 2020 22:43
@bryce-anderson
Copy link
Copy Markdown
Contributor Author

Fixed the order of static. I suppose I should have tested it before pushing. 🤦‍♂

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer normanmaurer merged commit 7b946a7 into netty:4.1 Mar 4, 2020
@normanmaurer
Copy link
Copy Markdown
Member

@bryce-anderson thanks a lot! nice work...

normanmaurer pushed a commit that referenced this pull request Mar 4, 2020
…Channel (#10075)

Motivation:

Under certain read patters the AbstractHttp2StreamChannel can fail to
flush, resulting in flow window starvation.

Modifications:

- Ensure we flush if we exit the `doBeginRead()` method.
- Account for the Http2FrameCodec always synchronously finishing writes
  of window update frames.

Result:

Fixes #10072
ihanyong pushed a commit to ihanyong/netty that referenced this pull request Jul 31, 2020
…Channel (netty#10075)

Motivation:

Under certain read patters the AbstractHttp2StreamChannel can fail to
flush, resulting in flow window starvation.

Modifications:

- Ensure we flush if we exit the `doBeginRead()` method.
- Account for the Http2FrameCodec always synchronously finishing writes
  of window update frames.

Result:

Fixes netty#10072
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.

AbstractHttp2StreamChannel fails to flush window update under certain read patterns

4 participants