data race in v1.0.9

[imkira]

Apparently it is not safe to read from the http.Transport as we get a data race bug when testing (go test -v -race -cpu=1,2,4 -covermode=atomic).

==================
WARNING: DATA RACE
Read by goroutine 81:
  github.com/aws/aws-sdk-go/aws/ec2metadata.NewClient()
      /home/ubuntu/go/src/github.com/aws/aws-sdk-go/aws/ec2metadata/service.go:54 +0xb5f
  github.com/aws/aws-sdk-go/aws/defaults.CredChain()
      /home/ubuntu/go/src/github.com/aws/aws-sdk-go/aws/defaults/defaults.go:94 +0x245
  github.com/aws/aws-sdk-go/aws/defaults.Get()
      /home/ubuntu/go/src/github.com/aws/aws-sdk-go/aws/defaults/defaults.go:34 +0xb14
  github.com/aws/aws-sdk-go/aws/session.New()
      /home/ubuntu/go/src/github.com/aws/aws-sdk-go/aws/session/session.go:54 +0x37
  example.com/sample.TestS3()
      /home/ubuntu/go/src/example.com/sample/s3_test.go:86 +0x82
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

Previous write by goroutine 79:
  sync/atomic.AddInt32()
      /tmp/workdir/go/src/runtime/race_amd64.s:255 +0xb
  sync.(*Mutex).Unlock()
      /tmp/workdir/go/src/sync/mutex.go:106 +0x55
  net/http.(*Transport).putIdleConn()
      /tmp/workdir/go/src/net/http/transport.go:412 +0xa8d
  net/http.(*persistConn).readLoop()
      /tmp/workdir/go/src/net/http/transport.go:997 +0xf0f

Goroutine 81 (running) created at:
  testing.RunTests()
      /tmp/workdir/go/src/testing/testing.go:561 +0xaa3
  testing.(*M).Run()
      /tmp/workdir/go/src/testing/testing.go:494 +0xe4
  main.main()
      example.com/sample/_test/_testmain.go:140 +0x384

Goroutine 79 (running) created at:
  net/http.(*Transport).dialConn()
      /tmp/workdir/go/src/net/http/transport.go:685 +0x11bf
  net/http.(*Transport).getConn.func4()
      /tmp/workdir/go/src/net/http/transport.go:549 +0x73
==================

The problem happens in:

aws-sdk-go/aws/ec2metadata/service.go

Line 54 in c4c1a1a

transport := *t

The fact that the transport is managed internally by net/http, makes me think it is unsafe and inappropriate any attempt to make a copy of it (especially when there are unexported fields and mutexes all around). Why not leave that to the user?

[jasdel]

Thanks for reporting this @imkira This looks to be caused by the change implemented for #504. I think you're correct the SDK should not be copying http.DefaultTransport. All this is done in order to provide shorter timeouts for the EC2RoleProvider in the CredentialsChain. Without the shorter timeout, using the SDK's default CredentialChain on a Non-EC2 host would take a over a minute to fail and return no credentials due to retries.

I think the #504 change and previous implementation is approaching the problem incorrectly. Instead of modifying the Transport the SDK should set the net/http#Client.Timeout instead. This will simplify the concept of EC2 Metadata custom timeouts, and only use this functionality if the client hasn't been modified. Adding a flag to turn off this logic all together is also probably needed.

[jasdel]

I've posted a PR addresses this data race and improves how the timeout override is configured. The user now has the option of disabling the override completely, in addition to using safe method of configuring the timeout if not disabled.

[imkira]

Thanks a lot for the prompt support @jasdel