potential data race in sentinel #9735
Conversation
| @@ -170,8 +167,18 @@ func (s *Sentinel) forkWatcher() { | |||
| return | |||
| } | |||
| if prevDigest != digest { | |||
| subs := s.subManager.subscriptions | |||
| for path, sub := range subs { | |||
| copy := func() map[string]*GossipSubscription { | |||
There was a problem hiding this comment.
so you are right there is a race, however this is not the correct fix, you still use the pointers which means that effectively you are not making a copy. i think a more elegant fix is to use sync.Map as the type in s.subManager.subscriptions
There was a problem hiding this comment.
hmm I just tried to resolve data race issue of concurrent r/w on map, but exactly they share the same GossipSubscription address. So the risk turns to be double close on GossipSubscription structure. That's why I introduce closeOnce.
For me more ideal way is to make subscription manipulations single thread like:
for {
select{
case <- addSubs:
case <- removeSubs:
case <- ticker.C:
}
}but we can see if really need this refactor.
There was a problem hiding this comment.
no, closeOnce will save you only from concurrent close
There was a problem hiding this comment.
the actual subscription object is thread-safe fyi, the closeOnce part is fine, but what I would like you to do is: instead of doing a map copy, to just use a sync.Map
This reverts commit a15603a.
|
I did not mean to approve it |
No description provided.