-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
local: fixes a data race in anti-entropy sync #12324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I think @kisunji has a lot more context here, so I'll wait on his review.
Does this depend on both previous fixes, or can we roll back some of that?
Parts of the prior fixes are still relevant so I think it makes more sense to roll forward. |
@@ -1083,31 +1083,35 @@ func (l *State) updateSyncState() error { | |||
continue | |||
} | |||
|
|||
// Make a shallow copy since we may mutate it below and other readers | |||
// may be reading it and we want to avoid a race. | |||
nextService := *ls.Service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still copies even if it's not going to mutate it for 1 of the 2 reasons below, but I sense that the clarity this provides in reading might be fine given the slightly increased allocations in the AE loop.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/582260. |
The race detector noticed this initially in
TestAgentConfigWatcherSidecarProxy
but it is not restricted to just tests.The two main changes here were:
agent/local
representation of a Service (for tags or VIPs) we clone those fieldsAddService
,AddCheck
, and related usingcopystructure
for now.