search

smartystreets / smartystreets-go-sdk

The official client libraries for accessing SmartyStreets APIs from Go.
https://smartystreets.com/docs/sdk/go
Apache License 2.0
20 stars 6 forks source link

Race condition: random source used in retry backoff strategy makes client unsafe for concurrent use from multiple goroutines #21

Closed mzimmerman closed 3 years ago

mzimmerman commented 3 years ago

================== WARNING: DATA RACE Read at 0x00c005bf9500 by goroutine 2551: math/rand.(rngSource).Uint64() /usr/local/go/src/math/rand/rng.go:239 +0x3e math/rand.(rngSource).Int63() /usr/local/go/src/math/rand/rng.go:234 +0x1d9 math/rand.(Rand).Int63() /usr/local/go/src/math/rand/rand.go:85 +0x125 math/rand.(Rand).Int31() /usr/local/go/src/math/rand/rand.go:99 +0x135 math/rand.(Rand).Int31n() /usr/local/go/src/math/rand/rand.go:131 +0x130 math/rand.(Rand).Intn() /usr/local/go/src/math/rand/rand.go:172 +0x59 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(RetryClient).backOff() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/retry_client.go:71 +0xc7 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(RetryClient).doBufferedPost() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/retry_client.go:54 +0x129 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(RetryClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/retry_client.go:34 +0x185 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(SigningClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/signing_client.go:28 +0xdc github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(BaseURLClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/base_url_client.go:27 +0x2ee github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(CustomHeadersClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/custom_header_client.go:19 +0x94 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(LicenseClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/license_client.go:25 +0xaa github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(HTTPSender).Send() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/http_sender.go:28 +0x7d github.com/smartystreets/smartystreets-go-sdk/us-extract-api.(Client).SendLookupWithContext() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/us-extract-api/client.go:33 +0x20a github.com/smartystreets/smartystreets-go-sdk/us-extract-api.(Client).SendLookup() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/us-extract-api/client.go:23 +0x1be

Previous write at 0x00c005bf9500 by goroutine 1650: math/rand.(rngSource).Uint64() /usr/local/go/src/math/rand/rng.go:239 +0x54 math/rand.(rngSource).Int63() /usr/local/go/src/math/rand/rng.go:234 +0x1d9 math/rand.(Rand).Int63() /usr/local/go/src/math/rand/rand.go:85 +0x125 math/rand.(Rand).Int31() /usr/local/go/src/math/rand/rand.go:99 +0x135 math/rand.(Rand).Int31n() /usr/local/go/src/math/rand/rand.go:131 +0x130 math/rand.(Rand).Intn() /usr/local/go/src/math/rand/rand.go:172 +0x59 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(RetryClient).backOff() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/retry_client.go:71 +0xc7 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(RetryClient).doBufferedPost() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/retry_client.go:54 +0x129 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(RetryClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/retry_client.go:34 +0x185 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(SigningClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/signing_client.go:28 +0xdc github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(BaseURLClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/base_url_client.go:27 +0x2ee github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(CustomHeadersClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/custom_header_client.go:19 +0x94 github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(LicenseClient).Do() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/license_client.go:25 +0xaa github.com/smartystreets/smartystreets-go-sdk/internal/sdk.(HTTPSender).Send() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/internal/sdk/http_sender.go:28 +0x7d github.com/smartystreets/smartystreets-go-sdk/us-extract-api.(Client).SendLookupWithContext() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/us-extract-api/client.go:33 +0x20a github.com/smartystreets/smartystreets-go-sdk/us-extract-api.(Client).SendLookup() /go/pkg/mod/github.com/smartystreets/smartystreets-go-sdk@v1.8.1/us-extract-api/client.go:23 +0x1be

mdwhatcott commented 3 years ago

Good find @mzimmerman . Am I correct in guessing you are using a single instance of a client concurrently from multiple goroutines?

A recent pull request introduced random 'jitter' into our backoff retry strategy via the math/rand package. I have just confirmed that the random Source currently being used is actually not safe for concurrent use. See the godoc:

https://golang.org/pkg/math/rand/#NewSource

NewSource returns a new pseudo-random Source seeded with the given value. Unlike the default Source used by top-level functions, this source is not safe for concurrent use by multiple goroutines.

A work-around for now would be to provide each goroutine with its own exclusive instance of a Client.

mzimmerman commented 3 years ago

Correct. Single instance over multiple goroutines. Is this correct usage or not? Seems like it would be easy enough to put a mutex around the random-jitter-source if there's no conflict otherwise. I've been using the library like this for at least a year and I'd rather not change.

mdwhatcott commented 3 years ago

@mzimmerman - We've never sought to limit the usage of a client to a single goroutine. This issue was unintentionally introduced by the pull request I mentioned earlier:

https://github.com/smartystreets/smartystreets-go-sdk/pull/18