fix: avoid stale and duplicate observers on concurrent Effect revalidation#24500
Open
heruan wants to merge 2 commits into
Open
fix: avoid stale and duplicate observers on concurrent Effect revalidation#24500heruan wants to merge 2 commits into
heruan wants to merge 2 commits into
Conversation
Two revalidations that both pass revalidate()'s first synchronized block while registrations is empty each add their own observers in the second block, leaving the effect with two observer generations. The test makes the interleaving deterministic with a private queueing dispatcher and a latch that freezes one revalidation inside the action -- after it has captured its observers but before its second synchronized block -- while the other revalidation runs to completion.
…ation revalidate() registers observers without holding the Effect monitor to avoid an ABBA deadlock with the signal tree lock. As a result several revalidation, invalidation and activation attempts can run concurrently, and the second synchronized block installed its observers unconditionally -- two overlapping attempts each added a generation, leaving duplicate and potentially stale listeners on the signal trees. Track a generation counter, bumped under the monitor by every attempt. An attempt installs its observers only if its captured generation is still current; otherwise it removes the observers it created. The most recent attempt therefore always wins, so the effect tracks the dependencies of its latest run and never accumulates duplicates. The activate() fast path uses the same check instead of its previous is-empty heuristic, which could keep a stale generation.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Effect.revalidate()registers signal-tree observers without holding the Effect monitor — this is deliberate, since holding the monitor across signal-tree calls caused the ABBA deadlock fixed in #24362. A consequence is that several revalidation / invalidation / activation attempts can run concurrently. The secondsynchronizedblock then did an unconditionalregistrations.addAll(...), so two overlapping attempts each added a generation of observers, leaving duplicate (and potentially stale) listeners on the signal trees.This is a follow-up to the post-merge review feedback on #24362: the
drainRegistrations()inrevalidate()'s first block closed one narrow leak but established no real invariant — nothing prevented another attempt from committing right after the block exits.Fix
A
generationcounter, bumped under the Effect monitor byrevalidate(),invalidate(), andactivate(). Each attempt capturesmyGeneration = ++generationin its first synchronized block and, in its second block, installs its observers only ifgeneration == myGeneration; otherwise it removes the observers it created (outside the monitor, preserving the deadlock fix). "Latest attempt wins" then holds for every interleaving, and stale attempts clean up after themselves instead of leaking.activate()'s fast path now uses the same generation check instead of its previous!registrations.isEmpty()heuristic, which could otherwise keep a stale generation and leave the effect tracking the wrong dependencies.Tests
concurrentRevalidations_doNotDoubleRegisterObservers— freezes one revalidation between its two synchronized blocks while another runs to completion; asserts a single observer generation. Fails on the pre-fix code.concurrentRevalidations_latestGenerationWins— proves the semantic property (the effect ends up tracking the latest run's dependency), which a naive "discard if non-empty" guard would get wrong.Both use a deterministic queueing dispatcher and latches (no timing flakiness). Full
EffectTest(49) and the whole signals package (648) pass.