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: awaitRequestCancel sometimes panics #19221

Closed
anthonybishopric opened this issue Feb 21, 2017 · 10 comments
Closed

x/net/http2: awaitRequestCancel sometimes panics #19221

anthonybishopric opened this issue Feb 21, 2017 · 10 comments

Comments

@anthonybishopric
Copy link

@anthonybishopric anthonybishopric commented Feb 21, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

Go 1.7.4 Linux

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build843429670=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

I'm using https://github.com/golang/oauth2 to exchange an authentication token for an access token from Google's OpenID provider. Sometimes, a spawned goroutine in the http/2 library panics in production, causing my entire web application to crash.

E  	/usr/local/go/src/net/http/h2_bundle.go:6189 +0x63b
 
E  created by net/http.(*http2clientConnReadLoop).handleResponse
 
E  	/usr/local/go/src/net/http/h2_bundle.go:5074 +0x1e9
 
E  net/http.(*http2clientStream).awaitRequestCancel(0xc42095f400, 0xc420825ef0)
 
E  	/usr/local/go/src/net/http/h2_bundle.go:2762 +0x53
 
E  net/http.(*http2pipe).CloseWithError(0xc42095f428, 0x0, 0x0)
 
E  	/usr/local/go/src/net/http/h2_bundle.go:2775 +0x20b
 
E  net/http.(*http2pipe).closeWithError(0xc42095f428, 0xc42095f478, 0x0, 0x0, 0x0)
 
E  	/usr/local/go/src/runtime/panic.go:500 +0x1a1
 
E  panic(0x8d04a0, 0xc420253fd0)
 
E  goroutine 128 [running]:
 
E  
 
E  panic: err must be non-nil

A program to reproduce this issue will need to set up a oauth2.Config object and then perform googleOAuthConfig.Exchange(ctx, authenticationCode) where ctx is the HTTP request's context and authenticationCode is the code received in the code= parameter in the callback URL.

The panic occurs in the routine spawned at h2_bundle.go:6811, inside awaitRequestCancel. It appears that when the Done() channel is closed, there is no corresponding error on ctx (line 5587).

What did you expect to see?

A valid exchange of the authentication token for an access token.

What did you see instead?

Sometimes, my server crashes outright.

@anthonybishopric anthonybishopric changed the title HTTP/2 await HTTP/2 awaitRequestCancel sometimes panics Feb 21, 2017
@anthonybishopric

This comment has been minimized.

Copy link
Author

@anthonybishopric anthonybishopric commented Feb 21, 2017

I believe this issue is present in Go 1.8 as well (or it's pebkac, which I would love to be the case)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 21, 2017

@bradfitz bradfitz added this to the Go1.9 milestone Feb 21, 2017
@bradfitz bradfitz changed the title HTTP/2 awaitRequestCancel sometimes panics x/net/http2: awaitRequestCancel sometimes panics Feb 21, 2017
@tombergan

This comment has been minimized.

Copy link
Contributor

@tombergan tombergan commented Feb 23, 2017

Do you have a minimal repro? The error seems impossible at first glance. Can you repro when running with the race detector?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 24, 2017

Are you using a custom context.Context implementation by chance?

@anthonybishopric

This comment has been minimized.

Copy link
Author

@anthonybishopric anthonybishopric commented Feb 24, 2017

Sorry for the delay. I will produce a runnable program when I get a chance, probably this weekend. Appreciate you taking the time to take a look!

The context.Context implementation is whatever shipped with the runtime. As far as I know, I didn't change it :)

@anthonybishopric

This comment has been minimized.

Copy link
Author

@anthonybishopric anthonybishopric commented Feb 24, 2017

Some more context, to be explicit: the oauth2 package doesn't do much of anything interesting with the context, other than store an http.Client to use for requests, and for the most part falls back to http.DefaultClient.

The error I get occurs while serving a request; it seems plausible that something having to do with handling the inbound request is racing with the outbound token exchange.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 21, 2017

Any luck reducing this to a repro? Any answer to @tombergan's question above?

@anthonybishopric

This comment has been minimized.

Copy link
Author

@anthonybishopric anthonybishopric commented Mar 21, 2017

I wrote a small service that connects to Google OpenID here https://github.com/anthonybishopric/go19221 however I can't get the case to repro just yet. I'm going to try to deploy this onto GKE and see if that changes things.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 22, 2017

I'm going to close this for now. File a new bug (referencing this one) if you get another crash and you're using the latest Go and have no races. Bonus points for minimal repro. :)

@bradfitz bradfitz closed this Mar 22, 2017
@anthonybishopric

This comment has been minimized.

Copy link
Author

@anthonybishopric anthonybishopric commented Mar 22, 2017

Yep, thanks and sorry. Will try to get a better repro case together.

@golang golang locked and limited conversation to collaborators Mar 22, 2018
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.