net/http: fix HTTP/2 idle pool tracing #34283

tmthrgd · 2019-09-13T10:43:17Z

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 #34282

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 below.

gopherbot · 2019-09-13T10:44:49Z

This PR (HEAD: 7e1634c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/195237 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-09-13T10:47:00Z

Message from Tom Thorogood:

Patch Set 1:

I'm presuming this needs some sort of test, but I'm not quite sure how to write one. It needs a HTTP/2 connection to come from the idle pool.

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-13T10:54:39Z

This PR (HEAD: 677ad13) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/195237 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-09-13T10:55:38Z

Message from Cuong Manh Le:

Patch Set 1:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-13T10:56:36Z

Message from Tom Thorogood:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-13T17:43:37Z

Message from Brad Fitzpatrick:

Patch Set 2:

Could you add a test for this?

You should be able to tweak/copy one of the existing ones.

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-14T01:16:28Z

This PR (HEAD: 2b7d66a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/195237 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-09-14T01:19:24Z

Message from Tom Thorogood:

Patch Set 3:

Patch Set 2:

Could you add a test for this?

You should be able to tweak/copy one of the existing ones.

The added test fails without this CL and passes with it. It also seems the simplest test I can think of,

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-16T12:09:30Z

Message from Michael Fraenkel:

Patch Set 3:

I believe TestTransportEventTrace_h2 should also be fixed to guarantee we do not log a GotConn for h2.

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-18T05:48:38Z

Message from Tom Thorogood:

Patch Set 3:

Patch Set 3:

I believe TestTransportEventTrace_h2 should also be fixed to guarantee we do not log a GotConn for h2.

I don't think an explicit test is needed, because TestTransportEventTrace_h2 already fails if the other w.pc.alt == nil check is missing.

--- FAIL: TestTransportEventTrace_h2 (0.00s)
transport_test.go:4289: expected substring "got conn: {" exactly once in output.
transport_test.go:4326: Output:
Getting conn for dns-is-faked.golang:36209 ...
DNS start: {Host:dns-is-faked.golang}
DNS done: {Addrs:[{IP:127.0.0.1 Zone:}] Err: Coalesced:false}
ConnectStart: Connecting to tcp 127.0.0.1:36209 ...
ConnectDone: connected to tcp 127.0.0.1:36209 =
tls handshake start
tls handshake done. ConnectionState = {772 true false 4865 h2 true [0xc000434b00] [] [] [] 0x691940 []}
err =
got conn: {Conn: Reused:true WasIdle:false IdleTime:0s}
got conn: {Conn:0xc000452000 Reused:false WasIdle:false IdleTime:0s}

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-18T11:36:22Z

Message from Michael Fraenkel:

Patch Set 3:

I don't think an explicit test is needed, because TestTransportEventTrace_h2 already fails if the other w.pc.alt == nil check is missing.

--- FAIL: TestTransportEventTrace_h2 (0.00s)

When I run it with or without your change I don't see a failure.
Is there some other tweak you had to do to cause the failure?
It should be fine to modify this test in the h2 case as your test case but right now it doesn't fail for me.

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-18T12:57:16Z

Message from Tom Thorogood:

Patch Set 3:

Patch Set 3:

I don't think an explicit test is needed, because TestTransportEventTrace_h2 already fails if the other w.pc.alt == nil check is missing.

--- FAIL: TestTransportEventTrace_h2 (0.00s)

When I run it with or without your change I don't see a failure.
Is there some other tweak you had to do to cause the failure?
It should be fine to modify this test in the h2 case as your test case but right now it doesn't fail for me.

Ah I thought you were referring to the other w.pc.alt check for freshly dialed connections. The added TestTransportTraceGotConnH2IdleConns covers this CL because it was too complicated adding it to TestTransportEventTrace.

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-18T13:44:19Z

Message from Michael Fraenkel:

Patch Set 3: Code-Review+1

Gotcha.

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-18T18:36:27Z

Message from Brad Fitzpatrick:

Patch Set 3: Run-TryBot+1 Code-Review+2

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-18T18:37:25Z

Message from Gobot Gobot:

Patch Set 3:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=f2842e36

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-09-18T18:48:47Z

Message from Gobot Gobot:

Patch Set 3: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/195237.
After addressing review feedback, remember to publish your drafts!

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 #34282 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 <[email protected]> Reviewed-by: Brad Fitzpatrick <[email protected]> Run-TryBot: Brad Fitzpatrick <[email protected]> TryBot-Result: Gobot Gobot <[email protected]>

gopherbot · 2019-09-18T18:54:43Z

This PR is being closed because golang.org/cl/195237 has been merged.

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 <[email protected]> Reviewed-by: Brad Fitzpatrick <[email protected]> Run-TryBot: Brad Fitzpatrick <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> (cherry picked from commit 582d519) Reviewed-on: https://go-review.googlesource.com/c/go/+/196579 Reviewed-by: Daniel Martí <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]> Run-TryBot: Daniel Martí <[email protected]>

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Sep 13, 2019

pc.alt == nil not w.pc.alt == nil

677ad13

Add net/http test

2b7d66a

gopherbot closed this Sep 18, 2019

net/http: fix HTTP/2 idle pool tracing #34283

net/http: fix HTTP/2 idle pool tracing #34283

Uh oh!

Conversation

tmthrgd commented Sep 13, 2019

Uh oh!

gopherbot commented Sep 13, 2019

Uh oh!

gopherbot commented Sep 13, 2019

Uh oh!

gopherbot commented Sep 13, 2019

Uh oh!

gopherbot commented Sep 13, 2019

Uh oh!

gopherbot commented Sep 13, 2019

Uh oh!

gopherbot commented Sep 13, 2019

Uh oh!

gopherbot commented Sep 14, 2019

Uh oh!

gopherbot commented Sep 14, 2019

Uh oh!

gopherbot commented Sep 16, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

gopherbot commented Sep 18, 2019

Uh oh!

Uh oh!