Fix navigation race condition with NavigateForResult #2967
Conversation
… before ViewModel loads Co-authored-by: kazo0 <[email protected]>
|
|
…cessing Co-authored-by: kazo0 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition where NavigateViewModelForResultAsync calls would hang indefinitely if the user triggered back navigation (via NavigationBar or hardware back button) before the target page's ViewModel finished loading.
Key Changes:
- Added
CompleteWithResultmethod toIResponseNavigatorinterface to allow external completion of pending navigation results - Implemented the method in
ResponseNavigator<TResult>with early-exit checks to avoid processing already-completed results - Modified
FrameNavigator.NavigatedBackAsyncto detect and complete any pendingResponseNavigatorbefore restoring the previous navigator
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Uno.Extensions.Navigation/IResponseNavigator.cs |
Added CompleteWithResult method to interface for completing pending navigation results from external callers |
src/Uno.Extensions.Navigation.UI/ResponseNavigator.cs |
Refactored result completion logic into new CompleteWithResult method with early-exit check, extracted from NavigateAsync |
src/Uno.Extensions.Navigation.UI/Navigators/FrameNavigator.cs |
Added logic to detect and complete pending ResponseNavigator during back navigation to prevent hung awaits |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Early return if already completed to avoid unnecessary processing | ||
| if (ResultCompletion.Task.Status == TaskStatus.Canceled || | ||
| ResultCompletion.Task.Status == TaskStatus.RanToCompletion) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The check on lines 55-56 for task completion status is not thread-safe. Between checking the status and calling ApplyResult, another thread could complete the task. This creates a TOCTOU (Time-Of-Check-Time-Of-Use) race condition.
Consider using a lock or interlocked operations to ensure atomicity, or rely on the fact that TrySetResult in line 94 is already thread-safe and will fail gracefully if the task is already completed. The duplicate check in ApplyResult (lines 83-84) provides some protection but doesn't eliminate the race window completely.
Alternatively, use an atomic flag to track completion state:
private int _isCompleted = 0;
public async Task CompleteWithResult(object? responseData)
{
if (Interlocked.CompareExchange(ref _isCompleted, 1, 0) == 1)
{
return;
}
// ... rest of the logic
}There was a problem hiding this comment.
The early return check is intentionally a performance optimization, not a thread-safety mechanism. The actual thread-safety is provided by TrySetResult in ApplyResult which is guaranteed to be atomic. The duplicate check in ApplyResult (lines 81-84) is the authoritative one. The early check in CompleteWithResultCore just avoids unnecessary type conversion work when the result is already set.
| // Complete any pending ResponseNavigator before restoring the previous navigator. | ||
| // This handles the case when back navigation occurs outside of the ResponseNavigator's | ||
| // NavigateAsync method (e.g., hardware back button, NavigationBar back button pressed | ||
| // before ViewModel is fully loaded). | ||
| var currentNavigator = Region.Services?.GetInstance<INavigator>(); | ||
| if (currentNavigator is IResponseNavigator responseNavigator) | ||
| { | ||
| var responseData = request.Route.ResponseData(); | ||
| await responseNavigator.CompleteWithResult(responseData); | ||
| } |
There was a problem hiding this comment.
The navigator restoration logic (line 90) is executed inside ApplyResult, which is called from CompleteWithResult. This means the navigator is restored before the TaskCompletionSource is set on line 94.
However, in FrameNavigator.NavigatedBackAsync (line 274-277), there's separate logic to restore the navigator from CurrentView. This could lead to a race condition where:
CompleteWithResultrestores the navigator (line 90)- Then
FrameNavigatorrestores it again fromCurrentView(line 277)
This could result in the wrong navigator being set. Consider coordinating this logic or ensuring that the navigator restoration happens only once in the correct place.
There was a problem hiding this comment.
This is actually intentional - the ResponseNavigator.ApplyResult restores the original navigator (the one that was wrapped), while FrameNavigator then restores the navigator stored on the previous page. These are typically the same navigator instance. The order is: (1) ResponseNavigator completes and restores its wrapped navigator, (2) FrameNavigator restores the navigator from the page we're navigating back to. This ensures the service provider has the correct navigator for the destination page.
| // Complete any pending ResponseNavigator before restoring the previous navigator. | ||
| // This handles the case when back navigation occurs outside of the ResponseNavigator's | ||
| // NavigateAsync method (e.g., hardware back button, NavigationBar back button pressed | ||
| // before ViewModel is fully loaded). | ||
| var currentNavigator = Region.Services?.GetInstance<INavigator>(); | ||
| if (currentNavigator is IResponseNavigator responseNavigator) | ||
| { | ||
| var responseData = request.Route.ResponseData(); | ||
| await responseNavigator.CompleteWithResult(responseData); | ||
| } |
There was a problem hiding this comment.
This fix addresses a race condition with NavigateForResult, but there are no tests covering this scenario. The testing infrastructure exists in the repository (see testing/TestHarness/TestHarness.UITest), but no tests verify the behavior when:
- A
NavigateForResultcall is awaited - Back navigation occurs before the target page's ViewModel finishes loading
- The awaited task should complete (not hang)
Consider adding a UI test that:
- Navigates from Page A to Page B using
NavigateViewModelForResultAsync - Triggers back navigation via hardware/UI back button before Page B's ViewModel is fully loaded
- Verifies that the awaited navigation call completes and doesn't hang
This would prevent regression of issue #2905.
| if (responseData is TResult data) | ||
| { | ||
| result = Option.Some(data); | ||
| } | ||
| else | ||
| { | ||
| result = Option.None<TResult>(); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (responseData is TResult data) | |
| { | |
| result = Option.Some(data); | |
| } | |
| else | |
| { | |
| result = Option.None<TResult>(); | |
| } | |
| result = responseData is TResult data ? Option.Some(data) : Option.None<TResult>(); |
There was a problem hiding this comment.
Applied in bd282c0 - simplified the if/else to a ternary operator.
|
@copilot While testing this in a local app I am getting this error: System.TypeLoadException: 'Method 'CompleteWithResult' in type 'Uno.Extensions.Navigation.ResponseNavigator`1' from assembly 'Uno.Extensions.Navigation.UI, Version=255.255.255.255, Culture=neutral, PublicKeyToken=null' does not have an implementation.' |
…CompleteWithResult Co-authored-by: kazo0 <[email protected]>
Fixed in bd282c0 by using explicit interface implementation ( |
GitHub Issue (If applicable): closes #2905
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When navigating from Page A to Page B with
NavigateViewModelForResultAsync, if the user triggers back navigation (via NavigationBar MainCommand or hardware back button) before Page B's ViewModel finishes loading, the awaited navigation call hangs forever. TheResponseNavigator'sTaskCompletionSourcenever completes because back navigation throughFrame_Navigatedevent bypasses theResponseNavigator.NavigateAsyncflow.What is the new behavior?
Back navigation now correctly completes pending
ResponseNavigatorinstances regardless of how the navigation is triggered. TheFrameNavigator.NavigatedBackAsyncmethod checks for any pendingResponseNavigatorin the service provider and completes it before restoring the previous navigator.Changes:
IResponseNavigator: AddedCompleteWithResult(object?)methodResponseNavigator<TResult>: ImplementedCompleteWithResultas an explicit interface implementation to fix TypeLoadException when the internal interface is used across assembly boundaries. Added privateCompleteWithResultCoremethod with early-exit check for thread safety.FrameNavigator.NavigatedBackAsync: Detects and completes pendingResponseNavigatoron back navigationPR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information
Internal Issue (If applicable):
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.