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

Remove races due to using plain pointers #3

Closed
mwitkow opened this issue Mar 22, 2016 · 1 comment · Fixed by #4
Closed

Remove races due to using plain pointers #3

mwitkow opened this issue Mar 22, 2016 · 1 comment · Fixed by #4

Comments

@mwitkow
Copy link
Owner

mwitkow commented Mar 22, 2016

Normal pflag and normal flag package use plain pointers. This is useful if the assumption is that no values will change past initialization of a server.

However, for dynamic flag updating, it causes races. In practice, everything is alright if running on GOMAXPROC=1, but if you go to multiple threads, stuff can break.

An example of running go test -race . in etcd directory:

michal:~/code/mygo/src/github.com/mwitkow/go-flagz/etcd/ (dynamic_flags*) $ go test -race .                                                     [15:42:47]
2016-03-22 15:42:59.314408 E | etcdserver: cannot monitor file descriptor usage (cannot get FDUsage on darwin)
==================
WARNING: DATA RACE
Read by goroutine 10:
  runtime.convT2E()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/iface.go:128 +0x0
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdate.func1()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:99 +0x56
  github.com/mwitkow/go-flagz/etcd_test.eventually()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:168 +0x92
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdate()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:100 +0x990
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Previous write by goroutine 18:
  flag.(*intValue).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:117 +0x86
  flag.(*FlagSet).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:363 +0x2ab
  github.com/mwitkow/go-flagz/etcd.(*Updater).watchForUpdates()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:175 +0x10f2

Goroutine 10 (running) created at:
  testing.RunTests()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:582 +0xae2
  github.com/stretchr/testify/suite.Run()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:102 +0x7a3
  github.com/mwitkow/go-flagz/etcd_test.TestUpdaterSuite()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:156 +0x595
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Goroutine 18 (running) created at:
  github.com/mwitkow/go-flagz/etcd.(*Updater).Start()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:86 +0x14d
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdate()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:94 +0x3f4
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc
==================
==================
WARNING: DATA RACE
Read by goroutine 46:
  runtime.convT2E()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/iface.go:128 +0x0
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdateRestoresGoodState.func3()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:141 +0x56
  github.com/mwitkow/go-flagz/etcd_test.eventually()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:168 +0x92
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdateRestoresGoodState()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:142 +0x11f2
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Previous write by goroutine 58:
  flag.(*float64Value).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:206 +0x7f
  flag.(*FlagSet).Set()
      /usr/local/Cellar/go/1.6/libexec/src/flag/flag.go:363 +0x2ab
  github.com/mwitkow/go-flagz/etcd.(*Updater).watchForUpdates()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:175 +0x10f2

Goroutine 46 (running) created at:
  testing.RunTests()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:582 +0xae2
  github.com/stretchr/testify/suite.Run()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:102 +0x7a3
  github.com/mwitkow/go-flagz/etcd_test.TestUpdaterSuite()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:156 +0x595
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc

Goroutine 58 (running) created at:
  github.com/mwitkow/go-flagz/etcd.(*Updater).Start()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater.go:86 +0x14d
  github.com/mwitkow/go-flagz/etcd_test.(*UpdaterTestSuite).Test_DynamicUpdateRestoresGoodState()
      /Users/michal/code/mygo/src/github.com/mwitkow/go-flagz/etcd/updater_test.go:118 +0x4ab
  runtime.call32()
      /usr/local/Cellar/go/1.6/libexec/src/runtime/asm_amd64.s:472 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.6/libexec/src/reflect/value.go:303 +0xcd
  github.com/stretchr/testify/suite.Run.func2()
      /Users/michal/code/mygo/src/github.com/stretchr/testify/suite/suite.go:94 +0x276
  testing.tRunner()
      /usr/local/Cellar/go/1.6/libexec/src/testing/testing.go:473 +0xdc
==================
PASS
Found 2 data race(s)
FAIL    github.com/mwitkow/go-flagz/etcd    3.588s
@mwitkow
Copy link
Owner Author

mwitkow commented Mar 22, 2016

In practice, it's not a big problem in production since it's highly unlikely for us to hit it, but it's still something really scary.

@mwitkow mwitkow mentioned this issue Mar 22, 2016
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 a pull request may close this issue.

1 participant