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

fix httptrace datarace #62

Merged
merged 1 commit into from
Jul 20, 2018
Merged

Conversation

t-yuki
Copy link
Contributor

@t-yuki t-yuki commented Jul 20, 2018

There is a data-race condition in xray/httptrace.go, because go's http client reuses idle connections aggressively.
DefaultTransport.getConn https://golang.org/src/net/http/transport.go#L990 dials new connection and reuses idleConn in parallel.

Without this change, the test will cause the below:

WARNING: DATA RACE
Write at 0x00c42051a7f8 by goroutine 129:
  github.com/aws/aws-xray-sdk-go/xray.(*Segment).Close()
      /home/t-yuki/go/src/github.com/aws/aws-xray-sdk-go/xray/segment.go:237 +0x248
  github.com/aws/aws-xray-sdk-go/xray.Capture.func1()
      /home/t-yuki/go/src/github.com/aws/aws-xray-sdk-go/xray/capture.go:23 +0x87
  github.com/aws/aws-xray-sdk-go/xray.Capture()
      /home/t-yuki/go/src/github.com/aws/aws-xray-sdk-go/xray/capture.go:48 +0x15a
  github.com/aws/aws-xray-sdk-go/xray.(*roundtripper).RoundTrip()
      /home/t-yuki/go/src/github.com/aws/aws-xray-sdk-go/xray/client.go:64 +0x1eb
  github.com/aws/aws-xray-sdk-go/xray.TestRoundTrip_reuse_datarace.func2()
      /home/t-yuki/go/src/github.com/aws/aws-xray-sdk-go/xray/client_test.go:215 +0x244

Previous read at 0x00c42051a7f8 by goroutine 32:
  github.com/aws/aws-xray-sdk-go/xray.(*HTTPSubsegments).ConnectStart()
      /home/t-yuki/go/src/github.com/aws/aws-xray-sdk-go/xray/httptrace.go:72 +0x6d
  github.com/aws/aws-xray-sdk-go/xray.NewClientTrace.func4()
      /home/t-yuki/go/src/github.com/aws/aws-xray-sdk-go/xray/httptrace.go:194 +0x69
  net.dialSingle()
      /home/t-yuki/go/tools/go/src/net/dial.go:537 +0xb43
  net.dialSerial()
      /home/t-yuki/go/tools/go/src/net/dial.go:515 +0x251
  net.(*Dialer).DialContext()
      /home/t-yuki/go/tools/go/src/net/dial.go:397 +0x8f1
  net.(*Dialer).DialContext-fm()
      /home/t-yuki/go/tools/go/src/net/http/transport.go:46 +0x99
  net/http.(*Transport).dial()
      /home/t-yuki/go/tools/go/src/net/http/transport.go:898 +0x322
  net/http.(*Transport).dialConn()
      /home/t-yuki/go/tools/go/src/net/http/transport.go:1143 +0x4ee
  net/http.(*Transport).getConn.func4()
      /home/t-yuki/go/tools/go/src/net/http/transport.go:957 +0x9f

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@luluzhao luluzhao merged commit 037b81b into aws:master Jul 20, 2018
@luluzhao
Copy link
Contributor

I merge this PR. Thank you again for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants