data race in lib/serf.UpdateTag
- Dominant language
- Go
- Stars
- 30.1k
- Forks
- 4.6k
- Avg merge
- 1d 18h
- Merged PRs (30d)
- 39
Description
There are actually 2 races here.
1. `Serf.SetTags` can race with itself. The godoc for the `Serf` struct claims that all methods should be safe for concurrent use, but there is no lock around `SetTags`, 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. (https://github.com/hashicorp/serf/issues/621)
2. Even if the above is fixed, the logic in `lib/serf.UpdateTag` is to first get the tags from `Serf.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 second `SetTags` 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
```
Contributor guide
Research direction
Review lib/serf/serf.go, especially UpdateTag, alongside the Serf.SetTags and Serf.GetTags call paths shown in the report. Run the relevant Go tests with the race detector and determine how concurrent tag updates should preserve all tags. Done means the race is no longer reported and concurrent updates do not lose tags.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100