Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Added InvokeOnMainThreadAsync to consolidate GoToAsync calls #15366

Closed

Conversation

MaxFelinger
Copy link

@MaxFelinger MaxFelinger commented May 11, 2022

Description of Change

Two of three GoToAsync calls were already wrapped by InvokeOnMainThreadAsync. We had a navigation issue because the third GoToAsync call in ShellNavigationManager is not surrounded by InvokeOnMainThreadAsync.
I do realise that this is a temporary fix and there should be a better fix. Nevertheless this consolidation is better than keeping it broken.

Issues Resolved

Fixes a navigation call that won't be executed otherwise.
We had this issue in the OnResume method. A specific scenario could be that the user has moved the app into the background while being at an important process step. The app could be terminated while it's in the background. To let the user proceed from where he has left off, we added a "ReentryService" that allows the user to jump to an important point where he left off. The ReentryService basically loads some stuff from SecureStorage (asynchronously) and invoked GoToAsync at its end.

I think we also had this issue when we used PushNotification, but I cannot remember the full scenario.

Platforms Affected

  • Core/XAML (all platforms)

Testing Procedure

I'm not sure how to test this case properly. I hope one of you reviewers knows how to test it.

@jfversluis
Copy link
Member

@PureWeen any thoughts about this one?

@jfversluis
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PureWeen
Copy link
Contributor

@MaxFelinger do you have a stack trace of your crash?

@MaxFelinger
Copy link
Author

MaxFelinger commented May 16, 2022

@PureWeen My bad it's not an exception that gets thrown, the navigation just won't be executed. I've adjusted the description of this PR.

To make things simple for reproduction purposes, someone could use this sample:

// Inside OnResume()
Task.Run(async () =>
{
    // Navigation won't be executed
    await Shell.Current.GoToAsync("AnyRelativeRoute");
    
    /* 
    Device.InvokeOnMainThreadAsync(() =>
    {
        // Navigation will be executed
        await Shell.Current.GoToAsync("AnyRelativeRoute");
    });
    */
});

@MaxFelinger

This comment was marked as off-topic.

@jfversluis

This comment was marked as off-topic.

@MaxFelinger
Copy link
Author

@jfversluis Thanks for the additional information.
Regarding the comment. It's actually not from me. I've copied the whole invocation part (including the comment) from above (lines 157 - 161).
// TODO get rid of this hack and fix so if there's a stack the current page doesn't display await Device.InvokeOnMainThreadAsync(() => { return _shell.CurrentItem.CurrentItem.GoToAsync(navigationRequest, queryData, animate, isRelativePopping); });
I was hoping @PureWeen would know how to UnitTest this scenario.

Reproducing this issue: Invoke an async method from OnResume and inside this async method invoke GoToAsync.

@PureWeen
Copy link
Contributor

@PureWeen My bad it's not an exception that gets thrown, the navigation just won't be executed. I've adjusted the description of this PR.

To make things simple for reproduction purposes, someone could use this sample:

// Inside OnResume()
Task.Run(async () =>
{
    // Navigation won't be executed
    await Shell.Current.GoToAsync("AnyRelativeRoute");
    
    /* 
    Device.InvokeOnMainThreadAsync(() =>
    {
        // Navigation will be executed
        await Shell.Current.GoToAsync("AnyRelativeRoute");
    });
    */
});

If you change that initial Task.Run to a Device.InvokeOnMainThread does it work? I think there's a few possible reasons why GotoAsync might not work when executed from a task pool thread vs the ui thread

@MaxFelinger
Copy link
Author

MaxFelinger commented Jun 25, 2022

@PureWeen yes, when I exchange Task.Run by Device.InvokeOnMainThreadAsync the navigation does work.

PS: The posted code is just a sample. The real code that we use in our project does async stuff (load from SecureStorage and a few more things) first and navigates afterwards. Sure we can surround the navigation call by Device.InvokeOnMainThreadAsync, but when I look into the Shell implementation and see that three GoToAsync invocations (very close to each other) and only one of them is not surrounded by Device.InvokeOnMainThreadAsync, I get curious and ask myself "Why aren't all three statements surrounded by Device.InvokeOnMainThreadAsync?".
Screen Shot 2022-06-25 at 7 20 21 AM

@jfversluis
Copy link
Member

Now that we're so close to the sunsetting of Xamarin.Forms unfortunately we won't be able to take this in anymore, we're really sorry about that. Nevertheless, thank you so much for your time and effort that you have put into this PR.

Please have a look at the evolution of Xamarin.Forms, .NET MAUI. A lot of development has been going on there. Hopefully this issue was already fixed in that codebase. If not, feel free to port this over to there.

Again, thank you so much for being a contributor and Xamarin.Forms user!

@jfversluis jfversluis closed this Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants