From 2fb3ff7eb64d6aff2945da378ea9b90b298bf464 Mon Sep 17 00:00:00 2001 From: Easley Date: Tue, 29 Sep 2026 20:14:32 +0200 Subject: [PATCH] fix: close reentrancy gap on the contended placement-lease path ViewPlacementCoordinator.EnterAsync's fast/reentrant paths write the holder marker in a plain (non-async) method body so it stays visible to the caller, but the genuinely-contended path delegated to an async EnterSlowAsync helper - and since AsyncLocal writes made inside a called async method never become visible to that method's own caller once it returns, a lease acquired via that slow path was never recognized as held by IsHeld() or by a nested reentrant EnterAsync call on the same region, reproducing both the spurious InvalidOperationException from RestorePlacementAsync's lease check and the original self-deadlock the reentrant fast path was built to fix. Replace the AsyncLocal depth counter with an AsyncLocal flow id established once per logical operation via a new EnsureFlow(), called synchronously at every entry point before EnterAsync (parent to child AsyncLocal flow always works; only child-to-caller propagation after a suspension does not). The "who currently holds this region" marker itself is a plain field on the shared per-region gate, not an AsyncLocal, so setting it from inside the async slow path is fine - only the flow id needs the caller-established treatment. Added ViewPlacementCoordinatorTests covering both the fast-path reentrant case and the previously-broken contended-then-reentrant case. Also: synchronize WeakUnloadObserver's ConditionalWeakTable read-modify-write (Add throws on a concurrent duplicate-key race), and set up the indicator host in RequestNavigateAsync before job scheduling and lease acquisition - not just inside CreateNavigateTask - so an error indicator can still be shown if either of those throw before reaching it. Co-Authored-By: Claude Sonnet 5 --- .../ViewPlacementService.cs | 3 + src/AsyncNavigation/RegionBase.cs | 2 + .../RegionNavigationService.cs | 5 + .../ViewPlacementCoordinator.cs | 95 +++++++++++++------ src/AsyncNavigation/WeakUnloadObserver.cs | 22 +++-- .../ViewPlacementCoordinatorTests.cs | 58 +++++++++++ 6 files changed, 147 insertions(+), 38 deletions(-) create mode 100644 tests/AsyncNavigation.Tests/ViewPlacementCoordinatorTests.cs diff --git a/src/AsyncNavigation.Floating/ViewPlacementService.cs b/src/AsyncNavigation.Floating/ViewPlacementService.cs index ae3dd25..a4fb3d2 100644 --- a/src/AsyncNavigation.Floating/ViewPlacementService.cs +++ b/src/AsyncNavigation.Floating/ViewPlacementService.cs @@ -30,6 +30,7 @@ public async Task FloatAsync( { ArgumentException.ThrowIfNullOrWhiteSpace(regionName); cancellationToken.ThrowIfCancellationRequested(); + using var flow = _placement.EnsureFlow(); using var lease = await _placement.EnterAsync(regionName, cancellationToken); if (navigationId.HasValue && @@ -137,6 +138,7 @@ public bool TryGetSession(Guid sessionId, out IFloatingViewSession? session) internal async Task RestoreCoreAsync(FloatingViewSession session, CancellationToken cancellationToken) { + using var flow = _placement.EnsureFlow(); using var lease = await _placement.EnterAsync(session.OriginRegionName, cancellationToken); if (!session.OriginRegion.TryGetTarget(out var originalRegion) || !_regionManager.TryGetRegion(session.OriginRegionName, out var region) || @@ -198,6 +200,7 @@ async Task RollbackContent() internal async Task CloseCoreAsync(FloatingViewSession session, CancellationToken cancellationToken) { + using var flow = _placement.EnsureFlow(); using var lease = await _placement.EnterAsync(session.OriginRegionName, cancellationToken); await session.Host.SetContentAsync(null, cancellationToken); try diff --git a/src/AsyncNavigation/RegionBase.cs b/src/AsyncNavigation/RegionBase.cs index bdd64e0..3567b44 100644 --- a/src/AsyncNavigation/RegionBase.cs +++ b/src/AsyncNavigation/RegionBase.cs @@ -72,6 +72,7 @@ Task IRegion.CanGoBackAsync() public async Task GoBackAsync(CancellationToken cancellationToken = default) { + using var flow = _placement?.EnsureFlow(); using var lease = _placement is null ? null : await _placement.EnterAsync(Name, cancellationToken); cancellationToken.ThrowIfCancellationRequested(); var navigationContext = _navigationHistory.GoBack() ?? throw new NavigationException("Cannot go back!"); @@ -102,6 +103,7 @@ Task IRegion.CanGoForwardAsync() public async Task GoForwardAsync(CancellationToken cancellationToken = default) { + using var flow = _placement?.EnsureFlow(); using var lease = _placement is null ? null : await _placement.EnterAsync(Name, cancellationToken); cancellationToken.ThrowIfCancellationRequested(); var navigationContext = _navigationHistory.GoForward() ?? throw new NavigationException("Cannot go forward!"); diff --git a/src/AsyncNavigation/RegionNavigationService.cs b/src/AsyncNavigation/RegionNavigationService.cs index 91eeae5..ae2f6c6 100644 --- a/src/AsyncNavigation/RegionNavigationService.cs +++ b/src/AsyncNavigation/RegionNavigationService.cs @@ -30,10 +30,15 @@ public RegionNavigationService(T regionPresenter, IServiceProvider serviceProvid public async Task RequestNavigateAsync(NavigationContext navigationContext, Action? onCompleted = null, bool coordinatePlacement = true) { navigationContext.ActivatedExternally = false; + // Set up the indicator host before anything that could throw (job scheduling, lease + // acquisition), so the catch block below can always show an error indicator - not just + // when the navigation got far enough to reach CreateNavigateTask's own Setup() call. + _regionIndicatorManager.Setup(navigationContext, _regionPresenter.IsSinglePageRegion); try { await _navigationJobScheduler.RunJobAsync(navigationContext, async context => { + using var flow = !coordinatePlacement || _placement is null ? null : _placement.EnsureFlow(); using var lease = !coordinatePlacement || _placement is null ? null : await _placement.EnterAsync(context.RegionName, context.CancellationToken); await CreateNavigateTask(context); diff --git a/src/AsyncNavigation/ViewPlacementCoordinator.cs b/src/AsyncNavigation/ViewPlacementCoordinator.cs index b75bf9c..40bfdc9 100644 --- a/src/AsyncNavigation/ViewPlacementCoordinator.cs +++ b/src/AsyncNavigation/ViewPlacementCoordinator.cs @@ -9,49 +9,71 @@ public sealed class ViewPlacementCoordinator private readonly ConcurrentDictionary _gates = new(); private readonly ConcurrentDictionary> _floating = new(ReferenceEqualityComparer.Instance); + // Identifies "this logical operation" so a nested EnterAsync call on the same region (e.g. a + // navigation callback that triggers a placement change) can recognize it already holds the + // lease instead of deadlocking against itself. Must be established by EnsureFlow() - called + // synchronously, before any await - at every top-level entry point (RequestNavigateAsync, + // GoBackAsync/GoForwardAsync, FloatAsync, RestoreCoreAsync, CloseCoreAsync): an AsyncLocal + // write made inside a called async method never becomes visible to that method's own caller + // once it returns (the compiler-generated state machine restores the caller's captured + // ExecutionContext on resume), even when the method never truly suspends. Establishing the id + // in the entry point's own frame sidesteps that entirely - the value then flows forward + // through every await *it* performs and everything it calls, which is the direction + // AsyncLocal always propagates correctly. + private readonly AsyncLocal _flowId = new(); + + /// + /// Marks the start of one logical placement operation so nested + /// calls on the same region can be recognized as reentrant. A no-op when already nested + /// inside an ancestor's flow (the ambient id is left alone). Callers should wrap their whole + /// operation: using var flow = coordinator.EnsureFlow(); using var lease = await + /// coordinator.EnterAsync(...);. + /// + public IDisposable EnsureFlow() + { + if (_flowId.Value != Guid.Empty) + return NoopScope.Instance; + _flowId.Value = Guid.NewGuid(); + return new FlowScope(this); + } + /// - /// Acquires the placement lease for a region. Reentrant within the same logical async - /// call chain: code that already holds the lease (e.g. a navigation callback that in turn - /// triggers a placement change on the same region) re-enters immediately instead of - /// deadlocking against itself. + /// Acquires the placement lease for a region. Reentrant within the same + /// scope: code that already holds the lease re-enters immediately instead of deadlocking + /// against itself. /// - /// - /// Deliberately not an `async` method: AsyncLocal writes made inside an async method are - /// invisible to its caller once it returns (the compiler-generated state machine restores - /// the caller's captured ExecutionContext), even when the method never actually suspends. - /// The uncontended/reentrant fast paths below write directly - /// in this plain method body so the mark stays visible to the caller's subsequent code - - /// which is what lets a nested re-entry on the same region see it. Only the genuinely - /// contended wait is delegated to an async helper, where that guarantee no longer holds. - /// public Task EnterAsync(string regionName, CancellationToken cancellationToken = default) { var gate = _gates.GetOrAdd(regionName, _ => new RegionGate()); - if (gate.Depth.Value > 0) - { - gate.Depth.Value++; + var flowId = _flowId.Value; + if (flowId != Guid.Empty && gate.HolderFlowId == flowId) return Task.FromResult(new Lease(gate, reentrant: true)); - } var wait = gate.Semaphore.WaitAsync(cancellationToken); if (wait.IsCompletedSuccessfully) { - gate.Depth.Value = 1; + gate.HolderFlowId = flowId; return Task.FromResult(new Lease(gate, reentrant: false)); } - return EnterSlowAsync(gate, wait); + return EnterSlowAsync(gate, wait, flowId); } - private static async Task EnterSlowAsync(RegionGate gate, Task wait) + private static async Task EnterSlowAsync(RegionGate gate, Task wait, Guid flowId) { await wait; - gate.Depth.Value = 1; + // A plain field write on the shared gate, not an AsyncLocal - ordinary object state isn't + // subject to the ExecutionContext isolation described above, so it's fine that this runs + // inside an async method the caller merely awaits. + gate.HolderFlowId = flowId; return new Lease(gate, reentrant: false); } - /// Returns whether the current logical async call chain already holds the placement lease for the region. - public bool IsHeld(string regionName) => - _gates.TryGetValue(regionName, out var gate) && gate.Depth.Value > 0; + /// Returns whether the current EnsureFlow scope already holds the placement lease for the region. + public bool IsHeld(string regionName) + { + var flowId = _flowId.Value; + return flowId != Guid.Empty && _gates.TryGetValue(regionName, out var gate) && gate.HolderFlowId == flowId; + } public bool IsFloating(IView view) => _floating.ContainsKey(view); @@ -73,7 +95,7 @@ public async Task TryActivateAsync(IView view, CancellationToken cancellat private sealed class RegionGate { public readonly SemaphoreSlim Semaphore = new(1, 1); - public readonly AsyncLocal Depth = new(); + public Guid HolderFlowId; } private sealed class Lease(RegionGate gate, bool reentrant) : IDisposable @@ -81,15 +103,26 @@ private sealed class Lease(RegionGate gate, bool reentrant) : IDisposable private RegionGate? _gate = gate; public void Dispose() { - var g = Interlocked.Exchange(ref _gate, null); - if (g is null) return; if (reentrant) - g.Depth.Value--; - else { - g.Depth.Value = 0; - g.Semaphore.Release(); + _gate = null; + return; } + var g = Interlocked.Exchange(ref _gate, null); + if (g is null) return; + g.HolderFlowId = Guid.Empty; + g.Semaphore.Release(); } } + + private sealed class NoopScope : IDisposable + { + public static readonly NoopScope Instance = new(); + public void Dispose() { } + } + + private sealed class FlowScope(ViewPlacementCoordinator owner) : IDisposable + { + public void Dispose() => owner._flowId.Value = Guid.Empty; + } } diff --git a/src/AsyncNavigation/WeakUnloadObserver.cs b/src/AsyncNavigation/WeakUnloadObserver.cs index 685cdb5..90cf391 100644 --- a/src/AsyncNavigation/WeakUnloadObserver.cs +++ b/src/AsyncNavigation/WeakUnloadObserver.cs @@ -14,15 +14,10 @@ internal sealed class WeakUnloadObserver // INavigationAware (e.g. after floating and docking a view back) replaces the previous // handler instead of stacking a second one that would fire alongside it. private static readonly ConditionalWeakTable> _subscriptions = new(); + private static readonly object _subscriptionsLock = new(); public static void Subscribe(INavigationAware navigationAware, Action onUnloadCallback) { - if (_subscriptions.TryGetValue(navigationAware, out var previousHandler)) - { - navigationAware.AsyncRequestUnloadEvent -= previousHandler; - _subscriptions.Remove(navigationAware); - } - var weakReference = new WeakReference(navigationAware); async Task HandleRequestUnloadAsync(object? sender, AsyncEventArgs args) @@ -40,7 +35,20 @@ async Task HandleRequestUnloadAsync(object? sender, AsyncEventArgs args) onUnloadCallback?.Invoke(target); } - _subscriptions.Add(navigationAware, HandleRequestUnloadAsync); + // TryGetValue/Remove/Add aren't individually atomic against each other, and + // ConditionalWeakTable.Add throws on a duplicate key - serialize the read-modify-write + // so two concurrent Subscribe calls for the same instance can't both pass the + // TryGetValue check and race on Add. + AsyncEventHandler? previousHandler; + lock (_subscriptionsLock) + { + _subscriptions.TryGetValue(navigationAware, out previousHandler); + if (previousHandler is not null) + _subscriptions.Remove(navigationAware); + _subscriptions.Add(navigationAware, HandleRequestUnloadAsync); + } + if (previousHandler is not null) + navigationAware.AsyncRequestUnloadEvent -= previousHandler; navigationAware.AsyncRequestUnloadEvent += HandleRequestUnloadAsync; } } diff --git a/tests/AsyncNavigation.Tests/ViewPlacementCoordinatorTests.cs b/tests/AsyncNavigation.Tests/ViewPlacementCoordinatorTests.cs new file mode 100644 index 0000000..dd551ed --- /dev/null +++ b/tests/AsyncNavigation.Tests/ViewPlacementCoordinatorTests.cs @@ -0,0 +1,58 @@ +namespace AsyncNavigation.Tests; + +public sealed class ViewPlacementCoordinatorTests +{ + [Fact] + public async Task EnterAsync_ReentrantFastPath_DoesNotDeadlock() + { + var coordinator = new ViewPlacementCoordinator(); + + using var flow = coordinator.EnsureFlow(); + using var outer = await coordinator.EnterAsync("region"); + + Assert.True(coordinator.IsHeld("region")); + + var inner = await coordinator.EnterAsync("region").WaitAsync(TimeSpan.FromSeconds(2)); + inner.Dispose(); + + Assert.True(coordinator.IsHeld("region")); + } + + [Fact] + public async Task EnterAsync_AfterContendedAcquire_IsHeldAndReentrancyBothWork() + { + var coordinator = new ViewPlacementCoordinator(); + + // Flow A takes the lease on the fast (uncontended) path and holds it open. + using var flowA = coordinator.EnsureFlow(); + var leaseA = await coordinator.EnterAsync("region"); + + // Flow B's EnterAsync must genuinely wait - it cannot complete until Flow A releases - + // exercising the async EnterSlowAsync path rather than the synchronous fast path. + var enterBTask = Task.Run(async () => + { + using var flow = coordinator.EnsureFlow(); + var lease = await coordinator.EnterAsync("region"); + + // Regression check: after acquiring via the contended/slow path, this same logical + // flow must be recognized as the holder (previously IsHeld() spuriously read false + // here because the depth marker never escaped the async helper that set it). + Assert.True(coordinator.IsHeld("region")); + + // Regression check: a nested EnterAsync call for the same region, from the same flow + // that just acquired it via the slow path, must be reentrant - not try to + // re-acquire the same semaphore and deadlock against itself. + var nested = await coordinator.EnterAsync("region").WaitAsync(TimeSpan.FromSeconds(2)); + nested.Dispose(); + + lease.Dispose(); + }); + + // Give Flow B's EnterAsync a moment to actually start waiting on the semaphore before + // releasing Flow A, so it exercises the contended path rather than racing to be first. + await Task.Delay(100); + leaseA.Dispose(); + + await enterBTask.WaitAsync(TimeSpan.FromSeconds(5)); + } +}