Fix CombinedUsage lock-order inversion with the SignalTree lock - #25178
Merged
Conversation
#25166) UsageTracker.CombinedUsage held its per-registration monitor while calling into signal trees, which can deadlock. When a tracked read spans two or more usages on the same tree (e.g. a cached/computed signal reading two nodes of one shared signal), registering a listener held the monitor while onNextChange acquired the dependency's SignalTree lock. A concurrent thread committing to that tree holds the tree lock and, from notifyObservers, invokes the first registered listener -> onChange, which then needs the monitor. The opposite acquire orders form an ABBA deadlock. The same applied to onChange invoking the downstream listener (which can read signals / register usages) while holding the monitor. This is a second, independent inversion from the CachedSignal one fixed for #25166: it survives that fix and triggers on the common multi-dependency case. Register the per-usage listeners and invoke the downstream listener without holding the monitor, matching what close() already does for removal. The monitor now only guards the small closed/cleanups state. onChange no longer serializes concurrent listener invocations, which the downstream listeners (Effect, CachedSignal, nested CombinedUsage) already tolerate. Add a deterministic regression test that races registration against commits; it deadlocks (ThreadMXBean-detected) on the old code and passes on the fix.
totally-not-ai
Bot
changed the title
Contributor
tltv
reviewed
Aug 11, 2026
The previous regression test relied on winning a narrow timing race across many iterations, so it did not reliably fail without the fix on all machines. Replace it with a deterministic reproduction: a control usage placed between two real usages on the same tree starts a committer mid-registration and waits (via thread state) until that committer holds the tree lock and is blocked entering the monitor, then lets registration proceed to the second usage, which needs that same tree lock. Without the fix this deadlocks on a single run (detected via a join timeout / ThreadMXBean); with the fix it passes.
totally-not-ai
Bot
changed the title
tltv
approved these changes
Aug 12, 2026
|
vaadin-bot
added a commit
that referenced
this pull request
Aug 12, 2026
…) (CP: 25.1) (#25190) This PR cherry-picks changes from the original PR #25178 to branch 25.1. --- #### Original PR description > ## Problem > > `UsageTracker.CombinedUsage` held its per-registration monitor while calling into signal trees, which can deadlock (ABBA): > > - When a tracked read spans two or more usages on the same tree (e.g. a cached/computed signal reading two nodes of one shared signal), registering a listener held the monitor while `onNextChange` acquired the dependency's `SignalTree` lock. > - A concurrent thread committing to that tree holds the tree lock and, from `notifyObservers`, invokes the first registered listener → `onChange`, which then needs the monitor. > > The same inversion applied to `onChange` invoking the downstream listener while holding the monitor, since that listener may itself read signals or register usages. > > This is a second, independent inversion from the `CachedSignal` one fixed for #25166: it survives that fix and triggers on the common multi-dependency case. > > ## Change > > - Register the per-usage listeners and invoke the downstream listener **without** holding the monitor, matching what `close()` already does for removal. The monitor now only guards the small `closed`/`cleanups` state. > - If the usage was closed while a registration was in flight, the cleanup is removed outside the monitor (removal also takes the tree lock). > - `onChange` no longer serializes concurrent listener invocations, which the downstream listeners (`Effect`, `CachedSignal`, nested `CombinedUsage`) already tolerate. > > ## Test > > Added a deterministic regression test in `UsageTrackerTest`. Rather than relying on winning a narrow timing race across many iterations, a control usage placed between two real usages on the same tree starts a committer mid-registration and waits (via thread state) until that committer holds the tree lock and is blocked entering the monitor, then lets registration proceed to the second usage, which needs that same tree lock. > > Without the fix this deadlocks on a single run (detected via a join timeout / `ThreadMXBean`); with the fix it passes. > > Fixes #25166 Co-authored-by: totally-not-ai[bot] <290682512+totally-not-ai[bot]@users.noreply.github.com> Co-authored-by: Tomi Virtanen <tltv@vaadin.com>
vaadin-bot
added a commit
that referenced
this pull request
Aug 12, 2026
…) (CP: 25.2) (#25189) This PR cherry-picks changes from the original PR #25178 to branch 25.2. --- #### Original PR description > ## Problem > > `UsageTracker.CombinedUsage` held its per-registration monitor while calling into signal trees, which can deadlock (ABBA): > > - When a tracked read spans two or more usages on the same tree (e.g. a cached/computed signal reading two nodes of one shared signal), registering a listener held the monitor while `onNextChange` acquired the dependency's `SignalTree` lock. > - A concurrent thread committing to that tree holds the tree lock and, from `notifyObservers`, invokes the first registered listener → `onChange`, which then needs the monitor. > > The same inversion applied to `onChange` invoking the downstream listener while holding the monitor, since that listener may itself read signals or register usages. > > This is a second, independent inversion from the `CachedSignal` one fixed for #25166: it survives that fix and triggers on the common multi-dependency case. > > ## Change > > - Register the per-usage listeners and invoke the downstream listener **without** holding the monitor, matching what `close()` already does for removal. The monitor now only guards the small `closed`/`cleanups` state. > - If the usage was closed while a registration was in flight, the cleanup is removed outside the monitor (removal also takes the tree lock). > - `onChange` no longer serializes concurrent listener invocations, which the downstream listeners (`Effect`, `CachedSignal`, nested `CombinedUsage`) already tolerate. > > ## Test > > Added a deterministic regression test in `UsageTrackerTest`. Rather than relying on winning a narrow timing race across many iterations, a control usage placed between two real usages on the same tree starts a committer mid-registration and waits (via thread state) until that committer holds the tree lock and is blocked entering the monitor, then lets registration proceed to the second usage, which needs that same tree lock. > > Without the fix this deadlocks on a single run (detected via a join timeout / `ThreadMXBean`); with the fix it passes. > > Fixes #25166 Co-authored-by: totally-not-ai[bot] <290682512+totally-not-ai[bot]@users.noreply.github.com> Co-authored-by: Tomi Virtanen <tltv@vaadin.com>
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
3 participants
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
UsageTracker.CombinedUsageheld its per-registration monitor while calling into signal trees, which can deadlock (ABBA):onNextChangeacquired the dependency'sSignalTreelock.notifyObservers, invokes the first registered listener →onChange, which then needs the monitor.The same inversion applied to
onChangeinvoking the downstream listener while holding the monitor, since that listener may itself read signals or register usages.This is a second, independent inversion from the
CachedSignalone fixed for #25166: it survives that fix and triggers on the common multi-dependency case.Change
close()already does for removal. The monitor now only guards the smallclosed/cleanupsstate.onChangeno longer serializes concurrent listener invocations, which the downstream listeners (Effect,CachedSignal, nestedCombinedUsage) already tolerate.Test
Added a deterministic regression test in
UsageTrackerTest. Rather than relying on winning a narrow timing race across many iterations, a control usage placed between two real usages on the same tree starts a committer mid-registration and waits (via thread state) until that committer holds the tree lock and is blocked entering the monitor, then lets registration proceed to the second usage, which needs that same tree lock.Without the fix this deadlocks on a single run (detected via a join timeout /
ThreadMXBean); with the fix it passes.Fixes #25166