hotfix(exporter): ensure thread-safe committee observer management #1883
Conversation
| c.committeesObserversMutex.Lock() | ||
| defer c.committeesObserversMutex.Unlock() | ||
|
|
||
| if msg.MsgType == spectypes.SSVConsensusMsgType { | ||
| // Check if the observer already exists | ||
| existingObserver := c.committeesObservers.Get(ssvMsg.GetID()) |
There was a problem hiding this comment.
isn't ttlcache already thread safe? why we need the mutex here then?
There was a problem hiding this comment.
Without the mutex, there’s a risk of race conditions where multiple goroutines might attempt to create and set the same observer simultaneously.
There was a problem hiding this comment.
this can be also solved by the cache's GetOrSet func, but involves creating the observer each time and sometimes for nothing. though it will be GC'd quickly if not used, there is no goroutine in the creation.
There was a problem hiding this comment.
You're right that GetOrSet avoids race conditions without a mutex, but it can lead to unnecessary observer creation, even if quickly garbage collected. The current mutex ensures observers are only created when needed, minimizing overhead for non-trivial operations. If the cost of unused creation is negligible, switching to GetOrSet could simplify the code. Let me know if we’d like to explore this direction further.
olegshmuelov
changed the title
| var nonCommitteeValidatorTTLs = map[spectypes.RunnerRole]int{ | ||
| spectypes.RoleCommittee: 64, | ||
| spectypes.RoleProposer: 4, | ||
| spectypes.RoleAggregator: 4, | ||
| //spectypes.BNRoleSyncCommittee: 4, | ||
| var committeeObserverValidatorTTLs = map[spectypes.RunnerRole]int{ | ||
| spectypes.RoleCommittee: 64, | ||
| spectypes.RoleProposer: 4, | ||
| spectypes.RoleAggregator: 4, |
There was a problem hiding this comment.
Maybe clarify that's denominated in slots (and not seconds or something)
| func (c *controller) calculateObserverTTL(roleType spectypes.RunnerRole) time.Duration { | ||
| ttlSlots := committeeObserverValidatorTTLs[roleType] | ||
| return time.Duration(ttlSlots) * c.beacon.GetBeaconNetwork().SlotDurationSec() | ||
| } |
There was a problem hiding this comment.
I think it's probably better to "panic" than "default to 0" in case there is a new type introduced and we forgot to add it to committeeObserverValidatorTTLs map (so we can discover this issue early)
This change resolves a data race by restructuring the management of
committeeObserverinstances