Skip to content

feat: add LeafLock to detect leaf-lock → tree-lock ordering violations (prototype) - #25171

Draft
totally-not-ai[bot] wants to merge 2 commits into
mainfrom
prototype-leaf-lock-ordering-assertion
Draft

feat: add LeafLock to detect leaf-lock → tree-lock ordering violations (prototype)#25171
totally-not-ai[bot] wants to merge 2 commits into
mainfrom
prototype-leaf-lock-ordering-assertion

Conversation

@totally-not-ai

@totally-not-ai totally-not-ai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Prototype for discussion — see #25166.

Why

Every one of the recurring shared-signal deadlocks is the same ABBA pair:

  • Thread A holds a component's own monitor and calls into a SignalTree, which then takes the tree lock.
  • Thread B holds the tree lock (notifyObservers invokes listeners under it) and then needs that component monitor.

These inversions are latent and timing-dependent, so they surface as rare CI flakes or production hangs rather than as test failures. A SignalTree cannot enumerate the arbitrary intrinsic monitors its callers happen to hold, so it can't defend itself.

What changed

LeafLock (new) — a component-level lock that records itself in a thread-local while held. Leaf locks make themselves known so the tree doesn't have to discover them.

SignalTree — asserts (under -ea) that no LeafLock is held when it acquires its own lock, ignoring reentrant tree re-locks. A leaf-lock → tree-lock violation now fails deterministically on the offending thread instead of deadlocking later under load.

UsageTracker.CombinedUsage — migrated to LeafLock, and its ordering fixed: it now registers the per-usage listeners and invokes the downstream listener without holding the leaf lock. This closes a real, reproduced ABBA (a registrar holding the CombinedUsage monitor and wanting the tree lock, versus a committer holding the tree lock and wanting the monitor) that survives the #25166 CachedSignal fix.

The thread-local is cleaned up properly (Sonar S5164): no withInitial(), so the read paths hit on every tree-lock acquisition never create a lingering entry; the deque is created lazily on the first lock() and the entry is removed when the last leaf lock on the thread is released.

Review notes

  • Main behavioral change to scrutinize: CombinedUsage.onChange now invokes the downstream listener outside the leaf lock, so concurrent fires are no longer serialized by it. This matches how the downstream listeners (Effect, CachedSignal, nested CombinedUsage) already tolerate concurrent/reentrant invocation — but it is the part worth a close look.
  • Effect and CachedSignal are not migrated to LeafLock yet; that's the follow-up if this direction is accepted.

Tests

  • The assertion fires deterministically on a leaf-lock → tree-lock violation.
  • CombinedUsage survives the register-vs-commit race.
  • Full signals suite (650 tests) stays green with the assertion active.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Test Results

 1 451 files  +1   1 452 suites  +1   1h 26m 30s ⏱️ -26s
10 413 tests +1  10 346 ✅ +1  67 💤 ±0  0 ❌ ±0 
10 849 runs  +1  10 781 ✅ +1  68 💤 ±0  0 ❌ ±0 

Results for commit fe6a260. ± Comparison against base commit 3d61fcf.

♻️ This comment has been updated with latest results.

@totally-not-ai totally-not-ai Bot changed the title prototype: leaf-lock ordering assertion for shared-signal ABBA deadlocks feat: add LeafLock to detect leaf-lock → tree-lock ordering violations (prototype) Aug 11, 2026
@Artur-
Artur- requested a review from Legioth August 11, 2026 11:22
@totally-not-ai
totally-not-ai Bot force-pushed the prototype-leaf-lock-ordering-assertion branch from 6dd1ba6 to 1a7a33d Compare August 11, 2026 13:17
@totally-not-ai
totally-not-ai Bot changed the base branch from main to fix/combinedusage-lock-order-inversion August 11, 2026 13:17
@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Aug 11, 2026
@github-actions github-actions Bot added +0.1.0 and removed +1.0.0 labels Aug 11, 2026
Base automatically changed from fix/combinedusage-lock-order-inversion to main August 12, 2026 08:42
…locks

Rebased on top of the CombinedUsage fix and reduced to just the abstraction:
the ordering fix and its deadlock regression test now live in that fix; this
commit only adds the enforcement mechanism and migrates the already-fixed call
site to it.

All the shared-signal deadlocks share one shape: a component holds its own
monitor while calling into a SignalTree (which takes the tree lock), while
another thread holds the tree lock (notifyObservers invokes listeners under it)
and then needs the component monitor.

LeafLock is a component-level lock that records itself in a thread-local while
held. SignalTree asserts (under -ea) that no LeafLock is held when it acquires
its own lock, ignoring reentrant tree re-locks. The tree cannot enumerate
arbitrary intrinsic monitors held by callers, so the leaf locks make themselves
known instead. This turns a latent, timing-dependent inversion into a
deterministic unit-test failure on any thread that violates the ordering.

CombinedUsage (already reordered by the fix) is migrated from its Object monitor
to a LeafLock as the first adopter, so the assertion guards it. Effect and
CachedSignal are not migrated yet; that is the follow-up if this direction is
accepted.

LeafLockPrototypeTest demonstrates the assertion firing on a leaf-lock ->
tree-lock violation.
@totally-not-ai
totally-not-ai Bot force-pushed the prototype-leaf-lock-ordering-assertion branch from 1a7a33d to f6d9424 Compare August 12, 2026 10:13
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants