Repository navigation
fix: close reentrancy gap on the contended placement-lease path - #56
Closed
NeverMorewd wants to merge 15 commits into
Closed
NeverMorewd wants to merge 15 commits into
NeverMorewd wants to merge 15 commits into
Conversation
- Make ViewPlacementCoordinator.EnterAsync reentrant per logical async call chain to fix a deadlock when a navigation callback (e.g. OnNavigatedToAsync) triggers a placement change on the same region; exposes IsHeld() for lease-contract enforcement. - Make WeakUnloadObserver.Subscribe idempotent so docking a floated view back no longer double-subscribes its unload handler. - Respect RegionPlacementItem.WasSelected on Attach instead of forcing activation, and only correct selection on a failed restore when it was actually disturbed. - Set up the indicator host unconditionally at the start of CreateNavigateTask so a failed or externally-activated navigation always has a usable IndicatorHost. - Add the missing Selected ?? Items.LastOrDefault() fallback to TabRegion/TabbedPageRegion.Capture() to match ItemsRegion behavior. - Fail fast in GetContentHost when an indicator host doesn't support content detachment, instead of silently corrupting the visual tree. - Enforce the documented placement-lease contract in RegionBase.RestorePlacementAsync via ViewPlacementCoordinator.IsHeld. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AvaloniaFloatingWindowHostFactory previously required Avalonia's classic desktop lifetime and threw NotSupportedException otherwise, so FloatAsync was unusable on browser/WASM and mobile single-view apps. It now selects the host per ApplicationLifetime: classic desktop still gets a real OS Window, while single-view apps get a new AvaloniaOverlayFloatingWindowHost that renders the floating content as a draggable, resizable in-page overlay panel (via Avalonia's OverlayLayer) with the same Dock/Close contract as the native window host. Verified with a new headless E2E test and manually in Sample.Avalonia.Browser via dotnet run + Chrome: float, drag, dock, and close all round-trip correctly with no console errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MainWindow.axaml previously hosted its own "Floating: current MainRegion view" button + description around <views:MainView />, so the demo only existed on classic desktop; MainView (the shared root used directly by Avalonia's single-view lifetime on browser/WASM, Android and iOS) never had it. Move the button and its FloatMainRegion_Click handler into MainView itself, and have MainWindow construct MainView in code and forward its IViewPlacementService instead of duplicating the header. Desktop, browser, Android and iOS now show the identical set of floating samples (MainRegion, TabRegion, ChildContentRegion, ItemsRegion, ListBoxRegion), with desktop's existing behavior as the reference. Verified in Sample.Avalonia.Browser via dotnet run + Chrome: the MainRegion float/dock button now shows on the home page and round-trips correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AsyncNavigation.E2E.Tests (Avalonia.Headless.XUnit) already exercises floating/docking/visual-state behavior but was never wired into CI - only tests/AsyncNavigation.Tests ran, and the sample jobs just build, never test. Add a test-e2e job mirroring the existing test job's 3-OS matrix. Retarget the test project from net10.0-windows to net10.0: nothing in it uses a Windows-only API, and Avalonia.Headless is cross-platform by design, so the Windows-only TFM was an unnecessary constraint blocking Linux/macOS runs. Verified locally (Windows) in both Debug and Release. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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<int> depth counter with an AsyncLocal<Guid> 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 <noreply@anthropic.com>
Owner
Author
|
Closing in favor of a clean PR: this branch's history predates PR #55's squash-merge, so comparing it against main again replays all 15 original commits and conflicts. Reopening the same fix from a fresh branch off current main instead. |
5 tasks done
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
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.
Summary
PR #55 was merged before this branch's last commit finished pushing, so the following fixes from that PR's final review pass never landed on
main. This PR carries just that missing commit over.EnterSlowAsync) set its reentrancy marker via anAsyncLocal<int>write made inside an async method — which never becomes visible to that method's own caller once it returns (the compiler-generated state machine restores the caller's capturedExecutionContexton resume, even when the method never truly suspends). This reproduced both the original self-deadlock the reentrant-lease fix was meant to prevent, and a spuriousInvalidOperationExceptionfromRestorePlacementAsync'sIsHeld()guard, whenever the first lease acquisition for a region happened to be genuinely contended. Replaced theAsyncLocal<int>depth counter with anAsyncLocal<Guid>flow id established once per logical operation via a newEnsureFlow(), called synchronously at every entry point beforeEnterAsync— since AsyncLocal always propagates correctly parent→child, only the old child→caller-after-suspension direction was broken. The "who holds this region" marker itself is a plain field on the shared per-region gate object, not an AsyncLocal, so writing it from inside the async slow path is fine.ConditionalWeakTableread-modify-write sequence (Addthrows on a concurrent duplicate-key race).RequestNavigateAsyncbefore job scheduling and lease acquisition — not just insideCreateNavigateTask— so an error indicator can still be shown if either of those throw before reaching it.Added
ViewPlacementCoordinatorTestscovering both the fast-path reentrant case and the previously-broken contended-then-reentrant case.Test plan
dotnet test tests/AsyncNavigation.Tests— 157 passeddotnet test tests/AsyncNavigation.E2E.Tests— 9 passedViewPlacementCoordinatorTests(2 tests) reproduce and verify the fix for the contended-lease self-deadlock/IsHeld gapsrc/AsyncNavigation.slnx) — 0 errorsCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com