search

mwitkow / go-flagz

Dynamic flag management for Go.
Apache License 2.0
206 stars 24 forks source link

Remove races due to using plain pointers #3

Closed mwitkow closed 8 years ago

mwitkow commented 8 years ago

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 commented 8 years ago

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.