Conversation
3ceb68f to
a1647fd
Compare
|
One more thing to be upfront about scope, this PR makes the in-memory Clear timestamps monotonic, but the L2 copy of the Clear is still last-writer-wins So L2 can still go backwards in a few cases:
But the impact is much smaller then the in-memory issue I am trying to fix via this PR. Running nodes with a backplane never re-read L2 and, thanks to this PR, keep the max they have seen. Only a node that starts fresh (or one without a backplane) can load the lower value, For the cases I described above that only matters for entries written between the two timestamps which is typically a window of milliseconds or the size of the clock skew I didn't try to fix this here because If you, as an author of the library, think it's worth it, I'd suggest we create an issue and try to come up with some nice solution in a separate PR not to widen the scope of the existing one and to properly discuss all the possible solutions with all the trade-offs |
Hi @jodydonetti! First of all, congrats on your new course on caching. And second, let me walk you through an issue I found☺️
ClearRemoveTimestampandClearExpireTimestampcan move backwards. When that happens, a node keeps serving entries that were already cleared, and with a backplane it does not recover on its own, because the field is loaded once and then only updated by notificationsThese changes make both fields monotonic: every writer now goes through a small helper that only moves the value forward
Why was it happening? Every writer of these two fields used
Interlocked.Exchange. That makes each write atomic, which is definitely better than a plain assignment, but it is unconditional: the last writer wins, even when its value is olderThe main case I found is with two nodes C1 and C2 sharing an L2 and a backplane, where C2 has just started (its fields are at
-1):0).Clear(). It writes the new timestampTto L2 and publishes it.T.Interlocked.Exchange(ref field, 0).Tis gone.>= 0and C2 has a backplane, so it is never reloadedFrom here C2 serves every pre-Clear entry until some later
Cleararrives. I reproduced it with deterministic tests:ClearRemoveDuringColdStartIsNotLostAsyncandClearExpireDuringColdStartIsNotLostAsyncThe other scenarios in short:
OutOfOrderClearRemoveNotificationsAreNotLostAsyncandOutOfOrderClearExpireNotificationsAreNotLostAsyncClearlowers a newer remote one: C2 receives a remoteClearwith timestampT, then calls its ownClear(), whosenowis smaller thanT, and the field goes down. This can happen with clock skew between nodes, or when the local call is paused right after taking its timestamp. Covered byLocalClearRemoveDoesNotUndoNewerRemoteClearAsyncandLocalClearExpireDoesNotUndoNewerRemoteClearAsyncThe fix I propose is a new helper,
FusionCacheInternalUtils.AdvanceTimestamp(ref long, long): a compare-and-swap loop that writes only if the new timestamp is greater than the current one, and returns the previous valueIt keeps what
Interlocked.Exchangeprovided (atomic write, previous value for the trace log) and adds the "only move forward" rule. A larger Clear timestamp covers everything a smaller one does, so keeping the maximum can never lose aClearAll writers of the two fields now use it, in both sync and async: the lazy loads, the raw-clear seed, the local
Clear, and the backplane handler. The "new Clear timestamp" trace log condition changed from!=to<, so it logs only when the value actually advanced