x/net/http2: push problems with Safari & Firefox #17777
Comments
|
Your fix doesn't look right (if they send ErrCodeProtocol, that suggests we're doing something else wrong), but thanks for the report. @tombergan, can you take a look? |
bradfitz
changed the title
|
There is also another issue here somewhere, especially with Firefox which we cannot get to work. We observe firefox coming to server once, doing push all fine, no problem. |
|
I agree with Brad, the proposed fix is not correct. If the client wants to disable push, they can do so by sending a SETTINGS frame with SETTINGS_ENABLE_PUSH disabled. Do you have a trace of a session where this is happening, especially if they are sending PROTOCOL_ERROR? Do Firefox/Safari send GOAWAY or just RST_STREAM? If they are sending RST_STREAM with PROTOCOL_ERROR or GOAWAY with anything except NO_ERROR, then we are likely doing something wrong. Alternatively, do you have a public website we could test against?
That sounds like a good thing! If Firefox has the resource cached, there's no reason to accept the push -- that just wastes bytes. It's an oversight that Chrome is not doing the same thing, and in fact, that is being fixed. |
|
Here are the relevant parts of the spec: https://tools.ietf.org/html/rfc7540#section-8
https://tools.ietf.org/html/rfc7540#section-8.2.2
|
|
Sadly i do not have a full trace, but this is whats happening in short: Apparently you're also not handling Cancel RSTs in the code i changed, they (i think) should also disable push. By the way, it would be awesome if http.Pusher interface had a function PushEnabled() bool, which just returns sc.pushEnabled, so the Application-Logic knows if push has been set upon connect or afterwards via settingsframe/rstframe. |
|
@fbettag, unfortunately, I cannot reproduce either the RST_STREAM or the PROTOCOL_ERROR with Firefox. Can you add some debugging printfs to check if the following line of code is reached, and if this line is reached, also print sc.maxStreamID? One hypothesis is that sc.state() is wrong for push. If the stream does not currently exist, then sc.state() needs to check against maxStreamID if streamID is odd or maxPushPromiseID if streamID is even.
This is not true. RST_STREAM does not disable push, it only cancels a single push. The server is free to push after getting a RST_STREAM. If the client wants to disable push, it does so with SETTINGS (see the comment above). I think you've found an actual bug. If you could rerun with printfs as described above, that would help verify the cause. Thanks! |
|
CL https://golang.org/cl/32755 mentions this issue. |
|
This seems to work now! Thanks I just came back to the computer and wanted to add your printfs :) sorry for the delay |
Fix sc.state, which was returning "idle" instead of "closed" if the max pushed stream ID happened to exceed the max client stream ID. This caused us to erroneously generate connection errors because we believed a state invariant had been violated when it had not. I also renamed maxStreamID to maxClientStreamID for clarity, since we need to track client-generated and server-generated stream IDs separately. Fixes golang/go#17777 Change-Id: Id3d5700d79cc699a267bd91d6ebace0770fa62fc Reviewed-on: https://go-review.googlesource.com/32755 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
What version of Go are you using (
go version)?go version devel +f4c7a12 Mon Oct 31 04:49:52 2016 +0000 darwin/amd64
What operating system and processor architecture are you using (
go env)?GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH=""
GORACE=""
GOROOT="/usr/local/Cellar/go/HEAD-f4c7a12/libexec"
GOTOOLDIR="/usr/local/Cellar/go/HEAD-f4c7a12/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/sg/9s7q5bgn25jbdkkdxwyxxf4m0000gn/T/go-build932596387=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
What did you do?
Use the newly written http2 support from @bradfitz and push resources to Safari and Firefox. If they don't like or wan't it, they will send RSTs. If you ignore them, they'll stop working with the server all together and close down the connection. -> broken
What did you expect to see?
Firefox/Safari continuing to talk to the http2 server.
What did you see instead?
Firefox/Safari closed the connection since we did not honor their RSTframes.
What did i do to fix it?
It's quite an easy fix. I've just added disabling push on the specified RST-Error-Codes for Firefox and Safari. There might be more for other browsers.
https://github.com/golang/net/pull/9/files
Other observations
Chrome apparently doesn't care if you honor it, it will simply work without a hitch.
The text was updated successfully, but these errors were encountered: