dskit
dskit copied to clipboard
Race condition in BasicLifecycler
There's a small race condition in BasicLifecycler
where concurrent calls to updateInstance
could cause a lost update if the ordering of calls changes after l.store.CAS
and before updating l.currInstanceDesc
. Some approaches for guarding against this are:
-
Pessimistic - We can broaden the
currState.Lock()
to include theCAS
call. -
Optimistic - We can store a
currVersion
along withcurrInstanceDesc
, updateCAS
to return the new version, and only updatecurrInstanceDesc
if the new version is >currVersion
.
We should also consider whether other methods that update l.currInstanceDesc
should be similarly guarded.
I was looking at this earlier today, and agree that race can occur. While we serialize all calls to updateInstance
in Running
state of BasicLifecycler
, we don't do that in Stopping
state: in this state we launch delegate's OnRingInstanceStopping
method in a goroutine, while we keep heartbeating in the main service goroutine.