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

net/http/httptrace: function called after RoundTrip returns an error #16957

tombergan opened this issue Sep 1, 2016 · 2 comments
Assignees
Milestone

Comments

@tombergan
Copy link
Contributor

@tombergan tombergan commented Sep 1, 2016

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

@quentinmit quentinmit commented Sep 6, 2016

/cc @bradfitz

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 21, 2016

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

@gopherbot gopherbot closed this in 23173fc Oct 21, 2016
@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.
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.