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.

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

okhttp: fix incorrect connection-level flow control handling at beginning of connection #6742

Merged
merged 6 commits into from Feb 27, 2020

Conversation

chrisschek
Copy link
Contributor

@chrisschek chrisschek commented Feb 22, 2020

Addressing #6685

Specifically, this addresses bugs that occur when the OkHttpChannelBuilder.flowControlWindow(int) setting is increased from its default value.

Two changes:

  1. On starting a connection, ensure the value of OkHttpChannelBuilder.flowControlWindow(int) is sent via Settings.INITIAL_WINDOW_SIZE. Also send a WINDOW_UPDATE after Settings to update the connection-level window.
  2. Always initialize the OutboundFlowController with an initialWindowSize of 65335 bytes per the http2 spec instead of using the inbound window size.

@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Feb 22, 2020

CLA Check
The committers are authorized under a signed CLA.

@ejona86
Copy link
Member

@ejona86 ejona86 commented Feb 24, 2020

Thanks for sending this out. I didn't send it out earlier because it clearly deserved some tests. I was on vacation for a bit over a week, thus the delay.

Thank you testing this and confirming the OutboundFlowController argument was garbage.

You'd need to sign the CLA for us to accept the PR. We can work on some tests and make a PR, if that is easiest.

@chrisschek
Copy link
Contributor Author

@chrisschek chrisschek commented Feb 25, 2020

I signed it

@chrisschek
Copy link
Contributor Author

@chrisschek chrisschek commented Feb 25, 2020

Hm, I signed the CLA (both on EasyCLA and the CNCF one linked in the contributing docs last week but it doesn't seem to be picking that up.

I took a stab at testing the changes:

  1. Was pretty straightforward for the OutboundFlowController argument; the outboundFlowControl_* tests seemed redundant after my fix so consolidated them into a single test.
  2. For sending INITIAL_WINDOW_SIZE in the first Settings frame, testing was trickier since that code doesn't normally run in the test. Not super happy with my end product having to change source code for the sake of testing, but seemed like a necessary evil.

@chrisschek chrisschek force-pushed the bugfix/okhttp-flowcontrol branch from 2fd5b20 to 8da3539 Compare Feb 25, 2020
@chrisschek
Copy link
Contributor Author

@chrisschek chrisschek commented Feb 25, 2020

Mystery solved – my commits had the wrong email address on them. :)

@chrisschek chrisschek marked this pull request as ready for review Feb 25, 2020
Copy link
Member

@ejona86 ejona86 left a comment

Looks pretty good. Just one thing to fix-up.

@@ -838,37 +868,34 @@ public void windowUpdateWithInboundFlowControl() throws Exception {

@Test
public void outboundFlowControl() throws Exception {
outboundFlowControl(INITIAL_WINDOW_SIZE);
Copy link
Member

@ejona86 ejona86 Feb 26, 2020

Choose a reason for hiding this comment

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

I think these three tests should still be around. They are trying to test that the outbound flow controller observes the flow control given to it. Since passing windowSize to startTransport() no longer works, these need to get something like frameHandler().settings(false, new Settings()); (but with the INITIAL_WINDOW_SIZE set to windowSize) to change the window size available.

Copy link
Contributor Author

@chrisschek chrisschek Feb 26, 2020

Choose a reason for hiding this comment

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

Done. I initially thought those cases were already covered by outboundFlowControlWithInitialWindowSizeChange(), but realized it wasn't testing where the initial window size is changed before the stream is started.

@ejona86 ejona86 added the kokoro:run label Feb 26, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label Feb 26, 2020
@chrisschek chrisschek force-pushed the bugfix/okhttp-flowcontrol branch from 66b5856 to 671dd33 Compare Feb 26, 2020
@ejona86 ejona86 added the kokoro:run label Feb 27, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run label Feb 27, 2020
@ejona86 ejona86 requested a review from voidzcy Feb 27, 2020
@ejona86
Copy link
Member

@ejona86 ejona86 commented Feb 27, 2020

@chrisschek, thank you! I was on vacation last week so I'm happy that you worked on this.

Sorry you were bit by this silly bug!

@ejona86 ejona86 added the TODO:backport label Feb 27, 2020
@chrisschek
Copy link
Contributor Author

@chrisschek chrisschek commented Feb 27, 2020

@ejona86 not a problem, happy to contribute :)

@ejona86 ejona86 merged commit bf2a66c into grpc:master Feb 27, 2020
13 checks passed
voidzcy added a commit that referenced this issue Feb 27, 2020
…ning of connection (v1.28.x backport)

Specifically, this addresses bugs that occur when the `OkHttpChannelBuilder.flowControlWindow(int)` setting is increased from its default value.

Two changes:
1. On starting a connection, ensure the value of `OkHttpChannelBuilder.flowControlWindow(int)` is sent via Settings.INITIAL_WINDOW_SIZE. Also send a WINDOW_UPDATE after Settings to update the connection-level window.
2. Always initialize the `OutboundFlowController` with an initialWindowSize of 65335 bytes per the [http2 spec](https://http2.github.io/http2-spec/#InitialWindowSize) instead of using the inbound window size.

Fixes #6685
Backport of #6742
@ejona86 ejona86 removed the TODO:backport label Mar 9, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants