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

Jump to bottom

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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Go1.9

Comments

@anthonybishopric
Copy link

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
Copy link
Author

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
Copy link
Contributor

/cc @tombergan

@bradfitz bradfitz added this to the Go1.9 milestone Feb 21, 2017
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 21, 2017
@bradfitz bradfitz changed the title HTTP/2 awaitRequestCancel sometimes panics x/net/http2: awaitRequestCancel sometimes panics Feb 21, 2017
@tombergan
Copy link
Contributor

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

@bradfitz
Copy link
Contributor

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

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 24, 2017
@anthonybishopric
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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

@bradfitz bradfitz added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Mar 21, 2017
@anthonybishopric
Copy link
Author

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
Copy link
Contributor

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. :)

@anthonybishopric
Copy link
Author

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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
4 participants
@bradfitz @anthonybishopric @gopherbot @tombergan