x/net/http2: hung request to vision.googleapis.com (bad flow control?) #16612
Comments
|
Okay, sorry, my fault. I see the problem. It's a flow control issue, but it's not because of https://golang.org/cl/25382. It's actually because of the mod_h2 workaround https://golang.org/cl/25362 (#16519). Google is advertising a 1MB initial window size for streams, but because we no longer wait to hear that, we start out assuming it's 64KB and never adjust our limit by (1MB-64KB) when we see the SETTINGS update by the peer. There's actually a TODO to do this: err := f.ForeachSetting(func(s Setting) error {
switch s.ID {
case SettingMaxFrameSize:
cc.maxFrameSize = s.Val
case SettingMaxConcurrentStreams:
cc.maxConcurrentStreams = s.Val
case SettingInitialWindowSize:
// TODO: error if this is too large.
// TODO: adjust flow control of still-open
// frames by the difference of the old initial
// window size and this one.
cc.initialWindowSize = s.Val... but it was super low priority when we read the SETTINGS at start-up, because I've never seen a server change the initial flow control setting on a connection after the initial settings. But it's super important to do that TODO if we're starting to do send requests (e.g. for Apache) before we wait for the initial SETTINGS from the peer. It's easy enough to roll forward and fix this, which I understand people might object to, but rolling back is also not great (there have been a few h2 fixes), and cherry picking fixes is also something I don't think will be too great if a number of them are inter-dependent. I'd prefer x/net and std to match as much as possible. I'll send out a fix tomorrow morning and people can evaluate it. |
|
Would this warrant another RC? |
|
Well, we already have to do another RC for two other reasons, so... |
|
What's the most conservative route? My gut feeling is that we fix it, and delay Go 1.7. I'd rather a solid but delayed 1.7 than to release with a flawed client that can't properly communicate with commonly used http2 servers (Google and Apache). |
|
I have a fix. It's not scary. Will mail it out shortly. |
|
CL https://golang.org/cl/25508 mentions this issue. |
The http2 spec says: > When the value of SETTINGS_INITIAL_WINDOW_SIZE changes, a receiver > MUST adjust the size of all stream flow-control windows that it > maintains by the difference between the new value and the old value. We didn't do this before, and it never mattered until https://golang.org/cl/25362 for golang/go#16519 because we always knew the peer's initial window size. Once we started writing request bodies before hearing the peer's setting (and thus assuming 64KB), it became very important that this TODO was done. Should've done it earlier. More details in the bug. Updates golang/go#16612 (fixes after bundle into std) Change-Id: I0ac0280bdd5f6e933ad82f8c9df3c4528295bac2 Reviewed-on: https://go-review.googlesource.com/25508 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Chris Broadfoot <cbro@golang.org>
|
CL https://golang.org/cl/25543 mentions this issue. |
Updates bundled http2 to x/net/http2 git rev 075e191 for: http2: adjust flow control on open streams when processing SETTINGS https://golang.org/cl/25508 Fixes golang#16612 Change-Id: Ib0513201bff44ab747a574ae6894479325c105d2 Reviewed-on: https://go-review.googlesource.com/25543 Run-TryBot: Chris Broadfoot <cbro@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Chris Broadfoot <cbro@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
I narrowed the issue to c558a53.
I suspect it's caused by https://golang.org/cl/25382.
Easiest way to reproduce... install Cloud SDK, do a
gcloud auth login, then:The text was updated successfully, but these errors were encountered: