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: function called after RoundTrip returns an error #16957

Closed
tombergan opened this issue Sep 1, 2016 · 2 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@tombergan
Copy link
Contributor

The documentation for httptrace says

// Functions may be
// called concurrently from different goroutines, starting after the
// call to Transport.RoundTrip and ending either when RoundTrip
// returns an error, or when the Response.Body is closed.

However, if RoundTrip is canceled or times out before t.dialConn completes, ConnectStart can be called concurrently with code that runs after RoundTrip returns an error.
https://golang.org/src/net/http/transport.go#L880

Here is a sample race found by TSAN:

WARNING: DATA RACE
Read at 0x00c420204418 by goroutine 98:
  [in my code that reads the final trace struct after RoundTrip has returned an error]

Previous write at 0x00c420204418 by goroutine 100:
  [in my implementation of ConnectStart]
  net.dialSingle()
      go/gc/src/net/dial.go:511 +0xde5
  net.dialSerial()
      go/gc/src/net/dial.go:489 +0x245
  net.(*Dialer).DialContext()
      go/gc/src/net/dial.go:371 +0x977
  [some frames redacted]
  net/http.(*Transport).dial()
      go/gc/src/net/http/transport.go:821 +0xe3
  net/http.(*Transport).dialConn()
      go/gc/src/net/http/transport.go:962 +0x242a
  net/http.(*Transport).getConn.func4()
      go/gc/src/net/http/transport.go:880 +0xa2

I'm not sure if this is a documentation bug or a code bug. The only way to implement the documentation faithfully is to either (a) prevent calling hooks after the conditions specified in the doc have been reached, or (b) wait for the background t.dialConn to complete before returning from RoundTrip with an error. The second solution doesn't seem practical. The first solution is doable, but do we really need a strict guarantee in the first place?

@quentinmit quentinmit added this to the Go1.8 milestone Sep 6, 2016
@quentinmit
Copy link
Contributor

/cc @bradfitz

@quentinmit quentinmit added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 7, 2016
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Oct 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants