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.

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

Correctly buffer multiple outbound streams if needed. #8694

Merged
merged 1 commit into from Jan 14, 2019

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Dec 28, 2018

Motivation:

In Http2FrameCodec we made the incorrect assumption that we can only have 1 buffered outboundstream as maximum. This is not correct and we need to account for multiple buffered streams.

Modifications:

  • Use a map to allow buffer multiple streams
  • Add unit test.

Result:

Fixes #8692.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Dec 28, 2018

Reported by @angn

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Dec 28, 2018

@netty-bot test this please

@bryce-anderson
Copy link
Contributor

bryce-anderson left a comment

It took me a few minutes to figure out why this would result in the stack trace in #8692, which I believe is that when we buffer more than one stream we skip setting the stream property but the connection does actually know about the stream. It might be nice to explain that a bit in the commit message. Then again, maybe it's obvious to everyone else.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Jan 14, 2019

@bryce-anderson added a comment in the code to explain why a map is needed.

Correctly buffer multiple outbound streams if needed.
Motivation:

In Http2FrameCodec we made the incorrect assumption that we can only have 1 buffered outboundstream as maximum. This is not correct and we need to account for multiple buffered streams.

Modifications:

- Use a map to allow buffer multiple streams
- Add unit test.

Result:

Fixes #8692.

@normanmaurer normanmaurer force-pushed the buffered_stream_fix branch from c6fb0bb to e4a2d06 Jan 14, 2019

@normanmaurer normanmaurer merged commit 4155bc0 into 4.1 Jan 14, 2019

@normanmaurer normanmaurer deleted the buffered_stream_fix branch Jan 14, 2019

normanmaurer added a commit that referenced this pull request Jan 14, 2019

Correctly buffer multiple outbound streams if needed. (#8694)
Motivation:

In Http2FrameCodec we made the incorrect assumption that we can only have 1 buffered outboundstream as maximum. This is not correct and we need to account for multiple buffered streams.

Modifications:

- Use a map to allow buffer multiple streams
- Add unit test.

Result:

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