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: potential data race on a global variable #2171

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Conversation

pior
Copy link
Contributor

@pior pior commented Mar 4, 2022

Running go test -race on some integration tests with Sarama trigger a data race warning:

WARNING: DATA RACE
Read at 0x0001052f7940 by goroutine 11:
  github.com/Shopify/sarama.version()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/version.go:8 +0x34
  github.com/Shopify/sarama.(*Broker).Open.func1.1()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/broker.go:183 +0x44
  github.com/Shopify/sarama.(*Broker).Open.func1()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/broker.go:245 +0x1368
  github.com/Shopify/sarama.withRecover()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/utils.go:43 +0x54
Previous write at 0x0001052f7940 by goroutine 12:
  github.com/Shopify/sarama.version()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/version.go:16 +0xec
  github.com/Shopify/sarama.(*Broker).Open.func1.1()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/broker.go:183 +0x44
  github.com/Shopify/sarama.(*Broker).Open.func1()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/broker.go:245 +0x1368
  github.com/Shopify/sarama.withRecover()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/utils.go:43 +0x54
The traces from the goroutines

Goroutine 11 (running) created at:
  github.com/Shopify/sarama.(*Broker).Open()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/broker.go:172 +0x300
  github.com/Shopify/sarama.(*client).any()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:686 +0xf8
  github.com/Shopify/sarama.(*client).tryRefreshMetadata()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:879 +0xbc
  github.com/Shopify/sarama.(*client).RefreshMetadata()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:489 +0x1d8
  github.com/Shopify/sarama.NewClient()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:170 +0x438
[...]
Goroutine 12 (running) created at:
  github.com/Shopify/sarama.(*Broker).Open()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/broker.go:172 +0x300
  github.com/Shopify/sarama.(*client).any()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:686 +0xf8
  github.com/Shopify/sarama.(*client).tryRefreshMetadata()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:879 +0xbc
  github.com/Shopify/sarama.(*client).RefreshMetadata()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:489 +0x1d8
  github.com/Shopify/sarama.NewClient()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/client.go:170 +0x438
  github.com/Shopify/sarama.NewConsumerGroup()
      /Users/pior/pkg/mod/github.com/!shopify/sarama@v1.32.0/consumer_group.go:97 +0x48
[...]

This is the code lazily populating the version from the Go module info:
https://github.com/Shopify/sarama/blob/3f90bc71bae993d5cbb08dfbb06cd10a88969d42/version.go#L7-L20

Solution

I'm proposing to protect this global variable with a sync.Once

@pior pior marked this pull request as ready for review March 4, 2022 20:27
@pior pior changed the title Fix a potential data race on a global variable fix: potential data race on a global variable Mar 4, 2022
@pior pior requested a review from dnwe March 4, 2022 20:37
@dnwe dnwe added the fix label Mar 7, 2022
@dnwe
Copy link
Collaborator

dnwe commented Mar 7, 2022

@pior thanks!

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

Successfully merging this pull request may close these issues.

3 participants