search

uber / ringpop-go

Scalable, fault-tolerant application-layer sharding for Go applications
http://www.uber.com
MIT License
835 stars 83 forks source link

Fix: Race conditions #178

Closed thanodnl closed 8 years ago

thanodnl commented 8 years ago

This PR is to remove the races from ringpop-go and let travis actually fail when there are race conditions in ringpop.

mennopruijssers commented 8 years ago

Looks really good overall. Some small questions and nits. One important piece missing though is a unit test for the two event emitter implementations (SyncEventEmitter and ASyncEventEmitter). Would be nice to have a specific unit test for those two.

mennopruijssers commented 8 years ago

LGTM but another race found on travis:

WARNING: DATA RACE
Read by goroutine 709:
  github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock.(*internalTimer).Tick()
      /home/travis/gopath/src/github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock/clock.go:277 +0x37
  github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock.(*Mock).runNextTimer()
      /home/travis/gopath/src/github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock/clock.go:136 +0x349
  github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock.(*Mock).Add()
      /home/travis/gopath/src/github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock/clock.go:76 +0x12c
  github.com/uber/ringpop-go/swim.TestPartitionHealGetsTriggeredWhenTimePasses()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/heal_partition_test.go:302 +0x8fb
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:456 +0xdc
Previous write by goroutine 711:
  github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock.(*Mock).AfterFunc()
      /home/travis/gopath/src/github.com/uber/ringpop-go/vendor/github.com/benbjohnson/clock/clock.go:150 +0x67
  github.com/uber/ringpop-go/swim.(*updateRollup).RenewFlushTimer()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/update_rollup.go:135 +0x170
  github.com/uber/ringpop-go/swim.(*updateRollup).TrackUpdates()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/update_rollup.go:116 +0x228
  github.com/uber/ringpop-go/swim.(*memberlist).Update()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/memberlist.go:565 +0xde4
  github.com/uber/ringpop-go/swim.reincarnateNodes()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/heal_partition.go:122 +0x29e
  github.com/uber/ringpop-go/swim.AttemptHeal()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/heal_partition.go:52 +0x5af
  github.com/uber/ringpop-go/swim.(*discoverProviderHealer).Heal()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/heal_via_discover_provider.go:157 +0x881
  github.com/uber/ringpop-go/swim.(*discoverProviderHealer).Start.func1()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/heal_via_discover_provider.go:79 +0x200
Goroutine 709 (running) created at:
  testing.RunTests()
      /tmp/workdir/go/src/testing/testing.go:561 +0xaa3
  testing.(*M).Run()
      /tmp/workdir/go/src/testing/testing.go:494 +0xe4
  main.main()
      github.com/uber/ringpop-go/swim/_test/_testmain.go:160 +0x20f
Goroutine 711 (running) created at:
  github.com/uber/ringpop-go/swim.(*discoverProviderHealer).Start()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/heal_via_discover_provider.go:89 +0x8d
  github.com/uber/ringpop-go/swim.TestPartitionHealGetsTriggeredWhenTimePasses()
      /home/travis/gopath/src/github.com/uber/ringpop-go/swim/heal_partition_test.go:287 +0x60e
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:456 +0xdc
==================
    <autogenerated>:29: ✅ HandleEvent(mock.AnythingOfTypeArgument)
mennopruijssers commented 8 years ago

There are some todo's left, but besides that: LGTM!

motiejus commented 8 years ago

LGTM