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: push problems with Safari & Firefox #17777

Closed
fbettag opened this issue Nov 3, 2016 · 8 comments
Closed

x/net/http2: push problems with Safari & Firefox #17777

fbettag opened this issue Nov 3, 2016 · 8 comments
Assignees
Milestone

Comments

@fbettag
Copy link

@fbettag fbettag commented Nov 3, 2016

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.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Nov 3, 2016

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 bradfitz added this to the Go1.8 milestone Nov 3, 2016
@bradfitz bradfitz changed the title HTTP/2 should disable push when client asks/RSTs x/net/http2: push problems with Safari & Firefox Nov 3, 2016
@fbettag

This comment has been minimized.

Copy link
Author

@fbettag fbettag commented Nov 3, 2016

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.
As soon as you refresh, firefox has most of these pushes already cached and refuses to accept them.
Apparently if you then try just a single push, it closes down just like above.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Nov 3, 2016

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?

We observe firefox coming to server once, doing push all fine, no problem.
As soon as you refresh, firefox has most of these pushes already cached and
refuses to accept them.

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.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Nov 3, 2016

Here are the relevant parts of the spec:

https://tools.ietf.org/html/rfc7540#section-8

A client can request that server push be disabled, though this is negotiated for each hop independently. The SETTINGS_ENABLE_PUSH setting can be set to 0 to indicate that server push is disabled.

https://tools.ietf.org/html/rfc7540#section-8.2.2

If the client determines, for any reason, that it does not wish to receive the pushed response from the server or if the server takes too long to begin sending the promised response, the client can send a RST_STREAM frame, using either the CANCEL or REFUSED_STREAM code and referencing the pushed stream's identifier.

@fbettag

This comment has been minimized.

Copy link
Author

@fbettag fbettag commented Nov 4, 2016

Sadly i do not have a full trace, but this is whats happening in short:
2016/11/03 16:55:27 http2: server read frame RST_STREAM stream=12 len=4 ErrCode=CANCEL
2016/11/03 16:55:27 http2: server connection error from 1.3.3.7:1337: connection error: PROTOCOL_ERROR

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.

@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Nov 4, 2016

@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?
https://github.com/golang/net/blob/569280fa63be4e201b975e5411e30a92178f0118/http2/server.go#L1232

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.

Apparently you're also not handling Cancel RSTs in the code i changed, they (i think) should also disable push.

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!

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 4, 2016

CL https://golang.org/cl/32755 mentions this issue.

@fbettag

This comment has been minimized.

Copy link
Author

@fbettag fbettag commented Nov 4, 2016

This seems to work now! Thanks

I just came back to the computer and wanted to add your printfs :) sorry for the delay

@golang golang locked and limited conversation to collaborators Nov 4, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
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>
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.