[Android]Fix deadlock on modal navigation#32920
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an Android-specific deadlock issue where calling PopModalAsync immediately after PushModalAsync with only a Task.Yield() in between would cause the app to hang. The solution introduces a Navigated event in the ModalFragment that fires in OnResume() to ensure non-animated modal navigation completes properly, and applies TaskCreationOptions.RunContinuationsAsynchronously to both TaskCompletionSource instances to ensure continuations run in the expected order.
Key changes:
- Added
Navigatedevent toModalFragmentclass that fires inOnResume()lifecycle method - Applied
TaskCreationOptions.RunContinuationsAsynchronouslyto both modal Push and Pop TaskCompletionSource instances - Added comprehensive UI test to verify the fix prevents the deadlock
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Android.cs |
Core fix: Added Navigated event, OnResume() override, and TaskCreationOptions.RunContinuationsAsynchronously to prevent deadlock in modal navigation |
src/Controls/tests/TestCases.HostApp/Issues/Issue32310.cs |
Test reproduction page that demonstrates the deadlock scenario with PushModalAsync + Task.Yield() + PopModalAsync |
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue32310.cs |
UI test that verifies modal navigation completes without hanging |
Comments suppressed due to low confidence (1)
src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.Android.cs:207
- Consider using
TrySetResultinstead ofSetResultfor consistency with the Pop modal path (line 123, 137) and for better error handling. IfOnResume()is called multiple times or if there's any other race condition,SetResultwill throw anInvalidOperationException, whileTrySetResultwould gracefully handle the case where the result is already set.
void OnNavigated(object? sender, EventArgs e)
{
dialogFragment.Navigated -= OnNavigated;
animationCompletionSource.TrySetResult(true);
}
void OnAnimationEnded(object? sender, EventArgs e)
{
dialogFragment!.AnimationEnded -= OnAnimationEnded;
animationCompletionSource.TrySetResult(true);
} void OnNavigated(object? sender, EventArgs e)
{
dialogFragment.Navigated -= OnNavigated;
animationCompletionSource.SetResult(true);
}
void OnAnimationEnded(object? sender, EventArgs e)
{
dialogFragment!.AnimationEnded -= OnAnimationEnded;
animationCompletionSource.SetResult(true);
Updated `ModalNavigationManager.Android.cs` to improve modal navigation handling: - Removed unused `AndroidX.Core.View` namespace. - Added `_pendingNavigation` field to track pending navigation operations. - Modified `OnResume` to check `_pendingNavigation` and prevent redundant `Navigated` event invocations. - Ensured `_pendingNavigation` is reset after navigation completes.
|
@PureWeen didn't see that one, by bad. I added some comments on that PR and will close this one |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
This PR adds the
Navigatedevent insideDialogFragment. The issue happens because the Push/PopModal navigation returns too early and causes the hanging since it gets lost.Also added
TaskCreationOptions.RunContinuationsAsynchronouslyto bothTaskCompletionSourcein Pop and Push modal navigations. This will make the continuations run in the expected order.I kept the
AnimationEndedevent because they fire in different moments, so I believe it's better to keep it for animated navigations.Issues Fixed
This is a follow up of: #32853
Fixes #32310