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

ReportEvent() race condition #71

Merged
merged 3 commits into from
Aug 15, 2017
Merged

ReportEvent() race condition #71

merged 3 commits into from
Aug 15, 2017

Conversation

cfchou
Copy link
Contributor

@cfchou cfchou commented Aug 14, 2017

CircuitBreaker.ReportEvent() checks Circuit.open without acquiring a lock. It's possible to run into data race when other goroutines running for example CircuitBreaker.setOpen().

This problem can be observed by running the test I add with package quick:

(Checkout #346dd5d and increase -quickchecks if not seeing race condition.)

go test -race github.com/cfchou/hystrix-go/hystrix -test.v -test.run ^TestReportEventMultiThreaded$ -quickchecks 50
...
==================
WARNING: DATA RACE
Write at 0x00c420106dd0 by goroutine 81:
  github.com/cfchou/hystrix-go/hystrix.(*CircuitBreaker).setOpen()
      /Users/cfchou/Project/gopath/src/github.com/cfchou/hystrix-go/hystrix/circuit.go:150 +0x1c9
  github.com/cfchou/hystrix-go/hystrix.(*CircuitBreaker).IsOpen()
      /Users/cfchou/Project/gopath/src/github.com/cfchou/hystrix-go/hystrix/circuit.go:108 +0x21e
  github.com/cfchou/hystrix-go/hystrix.TestReportEvent.func1.1()
      /Users/cfchou/Project/gopath/src/github.com/cfchou/hystrix-go/hystrix/circuit_test.go:137 +0x2bd

Previous read at 0x00c420106dd0 by goroutine 64:
  github.com/cfchou/hystrix-go/hystrix.(*CircuitBreaker).ReportEvent()
      /Users/cfchou/Project/gopath/src/github.com/cfchou/hystrix-go/hystrix/circuit.go:173 +0x393
  github.com/cfchou/hystrix-go/hystrix.TestReportEvent.func1.1()
      /Users/cfchou/Project/gopath/src/github.com/cfchou/hystrix-go/hystrix/circuit_test.go:131 +0x170
...

@afex
Copy link
Owner

afex commented Aug 15, 2017

thanks very much for the fix and test!

@afex afex merged commit dea3708 into afex:master Aug 15, 2017
@afex afex mentioned this pull request Aug 15, 2017
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