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

x/net/http2: too much coupling between connection and stream window sizes #15882

Closed
snaury opened this issue May 29, 2016 · 1 comment
Closed

x/net/http2: too much coupling between connection and stream window sizes #15882

snaury opened this issue May 29, 2016 · 1 comment
Assignees
Milestone

Comments

@snaury
Copy link
Contributor

@snaury snaury commented May 29, 2016

I have not really reproduced the problem in the code yet, so this is based on pure source code analysis that I did a while ago (even before http2 was merged into Go 1.6), and since I was reminded of it recently and code didn't change much in that regard in my recent review I decided to finally report it.

There is too much coupling in go implementation of http2 server, which makes stream windows practically useless. The problem is that the only code path where server increments a connection window is in noteBodyRead, where both connection and stream windows are updated for the remote side. What this effectively means is that at any given time all received (but not yet read by user code) data on all streams combined cannot exceed the connection window, making only one of them dominating the flow control. And since by default connection and stream windows are equal and rather small a single stream can consume the whole connection window, making it impossible for clients to send data on all other stream.

A hypothetical problematic case would be a file uploading service, that does some database pre-processing before starting to read from Request.Body. If that pre-processing takes a long enough time it would block all other concurrent POST requests until connection window starts to finally be drained.

What's more it appears that this approach of coupling connection and stream windows does not even save memory, since newWriterAndRequest allocates the full initialWindowSize buffer, which means memory footprint is numberOfStreams * streamWindowSize anyway, so updating connection window immediately upon parsing data frames would not increase it.

As far as I could understand from the spec connection and stream windows are supposed to be separate, and one good example I've seen recently was http2 in grpc c++ implementation, where connection window is substantially larger than stream window, and the algorithm is also different, as far as I understand: connection window is used for connection-level parsing, decoupled from streams, and updates to client start when more than 3/4 of it is consumed.

Sorry for not providing a code sample to this behavior, I've been postponing this bug report for so long already because of it that I finally decided at least some bug report now would probably be better than no bug report in the foreseeable future again.

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone May 30, 2016
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Aug 20, 2016

Just looking at this again. The current open bug for this is #16512 (which arose from #16510).

Even though this one came first, I'm going to close this in favor of the more succinct #16512 because I don't think there's actually any violation of the flow control specs in our http2 implementation. Let me know if I'm reading this wrong, though.

@bradfitz bradfitz closed this Aug 20, 2016
@golang golang locked and limited conversation to collaborators Aug 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.