dskit icon indicating copy to clipboard operation
dskit copied to clipboard

Race condition in BasicLifecycler

Open jhalterman opened this issue 1 year ago • 1 comments

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 the CAS call.
  • Optimistic - We can store a currVersion along with currInstanceDesc, update CAS to return the new version, and only update currInstanceDesc if the new version is > currVersion.

We should also consider whether other methods that update l.currInstanceDesc should be similarly guarded.

jhalterman avatar Apr 19 '23 14:04 jhalterman

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.

pstibrany avatar Apr 19 '23 15:04 pstibrany