data race in lib/serf.UpdateTag
Opened this issue · 0 comments
dnephin commented
There are actually 2 races here.
Serf.SetTags
can race with itself. The godoc for theSerf
struct claims that all methods should be safe for concurrent use, but there is no lock aroundSetTags
, so this is not the case. I'm not sure if it should be the responsibility of serf or the caller to handle that locking. (hashicorp/serf#621)- Even if the above is fixed, the logic in
lib/serf.UpdateTag
is to first get the tags fromSerf.GetTags
, mutate the map, then set the tags. But there is no lock! So if two goroutines attempt to update the tags at the same time, the secondSetTags
may not include all the tags from the first, and some tags would have effectively been removed.
Write at 0x00c000e2a590 by goroutine 29:
github.com/hashicorp/serf/serf.(*Serf).SetTags()
/home/daniel/go/pkg/mod/github.com/hashicorp/serf@v0.9.5/serf/serf.go:620 +0xa4
github.com/hashicorp/consul/lib/serf.UpdateTag()
/home/daniel/pers/code/consul/lib/serf/serf.go:48 +0xbb
github.com/hashicorp/consul/agent/consul.(*Server).updateSerfTags()
/home/daniel/pers/code/consul/agent/consul/acl_server.go:89 +0x84
github.com/hashicorp/consul/agent/consul.(*Server).updateACLAdvertisement()
/home/daniel/pers/code/consul/agent/consul/acl_server.go:102 +0x709
github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership()
/home/daniel/pers/code/consul/agent/consul/leader.go:125 +0x6ce
Previous write at 0x00c000e2a590 by goroutine 387:
github.com/hashicorp/serf/serf.(*Serf).SetTags()
/home/daniel/go/pkg/mod/github.com/hashicorp/serf@v0.9.5/serf/serf.go:620 +0xa4
github.com/hashicorp/consul/lib/serf.UpdateTag()
/home/daniel/pers/code/consul/lib/serf/serf.go:48 +0xbb
github.com/hashicorp/consul/agent/consul.(*Server).updateSerfTags()
/home/daniel/pers/code/consul/agent/consul/acl_server.go:89 +0x84
github.com/hashicorp/consul/agent/consul.(*Server).updateACLAdvertisement()
/home/daniel/pers/code/consul/agent/consul/acl_server.go:102 +0x113
github.com/hashicorp/consul/agent/consul.(*Server).establishLeadership()
/home/daniel/pers/code/consul/agent/consul/leader.go:314 +0xd8
github.com/hashicorp/consul/agent/consul.(*Server).leaderLoop()
/home/daniel/pers/code/consul/agent/consul/leader.go:199 +0x10af
github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership.func1()
/home/daniel/pers/code/consul/agent/consul/leader.go:91 +0x79
Goroutine 29 (running) created at:
github.com/hashicorp/consul/agent/consul.NewServer()
/home/daniel/pers/code/consul/agent/consul/server.go:585 +0x3029
github.com/hashicorp/consul/agent/consul.newServer()
/home/daniel/pers/code/consul/agent/consul/server_test.go:296 +0x204
github.com/hashicorp/consul/agent/consul.testServerWithConfig.func1()
/home/daniel/pers/code/consul/agent/consul/server_test.go:262 +0xf3
github.com/hashicorp/consul/sdk/testutil/retry.run.func2()
/home/daniel/pers/code/consul/sdk/testutil/retry/retry.go:133 +0x71
Goroutine 387 (running) created at:
github.com/hashicorp/consul/agent/consul.(*Server).monitorLeadership()
/home/daniel/pers/code/consul/agent/consul/leader.go:89 +0x38b