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

net/http/httptrace: panic on GotConn [1.13 backport] #34285

Closed
gopherbot opened this issue Sep 13, 2019 · 10 comments
Closed

net/http/httptrace: panic on GotConn [1.13 backport] #34285

gopherbot opened this issue Sep 13, 2019 · 10 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 13, 2019

@cuonglm requested issue #34282 to be considered for backport to the next 1.13 minor release.

@gopherbot please consider this for backport 1.13

@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 17, 2019

@cuonglm Could you please provide a short rationale about why the backport might be needed to 1.13? See https://golang.org/wiki/MinorReleases, which describes the process.

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Sep 17, 2019

@toothrot This is a regression from 1.12, and bug introduce in 1.13. It should be backport to 1.13.1, otherwise, user code which built with go1.13 will be broken, example see loadimpact/k6#1153

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Sep 18, 2019

@mvdan @bradfitz Should this issue be backport?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 18, 2019

SGTM.

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Sep 19, 2019

@tmthrgd would you mind creating CL with the cherry pick.

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Sep 20, 2019

@cuonglm I'm not sure how to do that, sorry. If someone else wants to create the cherry pick CL that would be appreciated.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 20, 2019

Change https://golang.org/cl/196579 mentions this issue: [release-branch.go1.13] net/http: fix HTTP/2 idle pool tracing

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Sep 20, 2019

Thanks @ALTree, that was simple enough to follow.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Sep 25, 2019

Closed by merging a055bb9 to release-branch.go1.13.

@gopherbot gopherbot closed this Sep 25, 2019
gopherbot pushed a commit that referenced this issue Sep 25, 2019
CL 140357 caused HTTP/2 connections to be put in the idle pool, but
failed to properly guard the trace.GotConn call in getConn. dialConn
returns a minimal persistConn with conn == nil for HTTP/2 connections.
This persistConn was then returned from queueForIdleConn and caused the
httptrace.GotConnInfo passed into GotConn to have a nil Conn field.

HTTP/2 connections call GotConn themselves so leave it for HTTP/2 to call
GotConn as is done directly below.

Fixes #34285

Change-Id: If54bfaf6edb14f5391463f908efbef5bb8a5d78e
GitHub-Last-Rev: 2b7d66a
GitHub-Pull-Request: #34283
Reviewed-on: https://go-review.googlesource.com/c/go/+/195237
Reviewed-by: Michael Fraenkel <michael.fraenkel@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 582d519)
Reviewed-on: https://go-review.googlesource.com/c/go/+/196579
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
@bcmills bcmills modified the milestones: Go1.13.1, Go1.13.2 Sep 25, 2019
@katiehockman katiehockman modified the milestones: Go1.13.2, Go1.13.3 Oct 17, 2019
@golang golang locked and limited conversation to collaborators Oct 16, 2020
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
8 participants
You can’t perform that action at this time.