opened
Cheesebaron
username_0
### :sparkles: What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Bug fix

### :arrow_heading_down: What is the current behavior?
Deadlock on startup when running on UWP.

Currently when simply uncommenting https://github.com/MvvmCross/MvvmCross/blob/develop/Projects/Playground/Playground.Core/ViewModels/RootViewModel.cs#L179 or adding a simple `await Task.Delay(500)` in Initialize of the RootViewModel on UWP, then the App never finishes starting up.

It seems like Startup doesn't like content switching a lot, especially not switching back to UI thread.

### :new: What is the new behavior (if this is a feature change)?
Remove `GetAwaiter().GetResult()` which _will_ block as soon as you try to get back to the UI thread. It is the same as calling `.Result`. Since the method is already marked `async` I just await it instead.

Also I don't see any reason as to why everything needs to return on to the context it came from, so added a bit of `ConfigureAwait(false)`.

### :boom: Does this PR introduce a breaking change?
Hopefully not.

### :bug: Recommendations for testing
Reviewers please test other platforms than UWP too!

### :memo: Links to relevant issues/docs
#3209 
#2953
#3221

### :thinking: Checklist before submitting

- [x] All projects build
- [x] Follows style guide lines ([code style guide](https://github.com/MvvmCross/MvvmCross#code-style-guidelines))
- [ ] Relevant documentation was updated ([docs style guide](https://www.mvvmcross.com/documentation/contributing/mvvmcross-docs-style-guide))
- [x] Rebased onto current develop
GetAwaiter().GetResult() is a really bad idea in a async method
issue
created
nickrandolph
username_1
The point of calling GetResult is to force synchronouse behaviour here  - doing async work on startup will cause issues and random exceptions if it takes too long. 
Don't call ConfigureAwait(false) - this doesn't return from the method on a different thread, it just means that the remainder of the thread is done on a different thread. Little or no saving.
comment
created
Cheesebaron
username_0
It is fine for `StartupAsync` to call it with `GetAwaiter().GetResult()`, I understand that we want to block there. However, for `ApplicationStartup` to call `Navigate` like that is prone to break a lot. How come behavior is _different_ between implementing `MvxAppStart` that just takes a `ViewModel` contra the one with both `ViewModel` and `Parameter`? I don't see anything warrants this?
comment
created
nickrandolph
username_1
Off top of my head I don't know the answer. Coming back to the UWP blocking issue, the answer is actually to get off the UI thread and the jump back as required (or don't await your init logic)
comment
created
slodge
username_2
https://stackoverflow.com/a/5097066 <- MIT license
comment
created
elucidsoft
username_3
This needs an immediate fix, debating the finer points of how to do this properly should be something that can be done after a hotfix for this issue is applied. This is a pretty severe issue.
comment
created
nickrandolph
username_1
@username_3 no there is no excuse for implementing a patch that will just make the situation worse. From my understanding this is only an issue where an await is called within the Initalise of the first view model, or have I missed something? If this is the case, the workaround is to shift the awaitable logic to allow the first view to load
comment
created
elucidsoft
username_3
This is an issue if there's a blocking call of any kind in the initialize of the first viewmodel. It will hang on startup, what's worse is it can hang on startup randomly and in unpredictable ways. For instance I was loading a database, since that took a different amount of time 90% of the time the app loaded fine. 10% of the time people were seeing random hangs on startup of the app.  In the test case it's been simplified to demonstrate the issue.
comment
created
nickrandolph
username_1
And you can't move this logic so that it gets done after the initial navigation?
comment
created
elucidsoft
username_3
I have a workaround, but imagine how many people who use this framework will have this issue? Perhaps even unnoticed, seems pretty bad.
comment
created
nickrandolph
username_1
@username_3 in the short term we need to probably highlight this issue in the docs. Long term we might want to consider a synchronous navigation for the first navigation - we need to be explicit that async work should not be undertaken before the first view is shown. Particularly for XF delaying loading the first view will cause issues.
comment
created
flyingxu
username_4
`GetAwaiter().GetResult()` will block any further await calls with the same context unless the await calls are called with `ConfigureAwait(false)` (by giving up the context). See: https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html . 
The root cause of this issue is that `GetAwaiter().GetResult()` blocks the UI context and the `await Task.Delay()` can't get the context to return.

If yo search the whole MvvmCross lib code, there are only 2 `GetAwaiter().GetResult()` calls now. One is removed by this PR, the other one is mentioned [earlier](https://github.com/MvvmCross/MvvmCross/pull/3222#issuecomment-443078022) and I strongly suggest we remove it for good. If we keep the other `GetAwaiter().GetResult()`, we will find it blocks some other code sooner or later or we make all the await calls with `ConfigureAwait(false)`.
comment
created
flyingxu
username_4
oh, even everybody (the lib, the lib's user) use 'ConfigureAwait(false)', it's not guaranteed to fix the issue 100% because 'ConfigureAwait(false)' is not guaranteed to switch to a new context. 
Interesting reading:

https://stackoverflow.com/questions/53456754/configureawaitfalse-does-the-continuation-always-run-on-different-thread

Here comes my final suggestion: remove all `GetAwaiter().GetResult()` from UI thread.
comment
created
Cheesebaron
username_0
@username_1 it does yield a difference, if _all_ calls down the chain from the `StartAsync` have `ConfigureAwait(false)`. However, we cannot control that when a user doesn't do this in their `Initialize` method in their ViewModel.

We should look into the SO thread @username_2 posted. It seems interesting.
Alternatively we could maybe abuse the `MvxNotifyTask` a bit with a `TaskCompletionSource` to get a blocking behavior.

I'll have another stab at this soon.
comment
created
Cheesebaron
username_0
So. I think I've fixed this now. I've added a SynchronizationContext which is based on Stephen Toub's AsyncPump [[1](https://blogs.msdn.microsoft.com/pfxteam/2012/01/20/await-synchronizationcontext-and-console-apps/)][[2](https://blogs.msdn.microsoft.com/pfxteam/2012/01/21/await-synchronizationcontext-and-console-apps-part-2/)][[3](https://blogs.msdn.microsoft.com/pfxteam/2012/02/02/await-synchronizationcontext-and-console-apps-part-3/)]

This allows us to kick off all tasks that are in `StartAsync` on the current thread and run serially until completion. This is what the `SingleThreadSynchronizationContext` ensures by simply running each task that is queued within `StartAsync` in succession.
comment
created
nickrandolph
username_1
I've just run this up and walked through it a couple of times to get my head around it. It looks good to me and hopefully will resolve a bunch of startup related issues in the backlog - well done!
comment
created
ferrydeboer
username_5
This dosn't work yet because the MvxIosUIThreadDispatcher.IsOnMainThread returns false due to the SynchronizationContext being of a different type. This is fairly easy to reproduce using a custom AppStart with the following implementation for NavigateToFirstViewModel:
```        
protected override async Task NavigateToFirstViewModel(object hint = null)
{
    await Task.Delay(1500);
    await NavigationService.Navigate<RootViewModel>();
}
```
I validated this by modifying the MvxIosUIThreadDispatcher.IsOnMainThread:
```
        public override bool IsOnMainThread
        {
            get
            {
                if (_uiSynchronizationContext == SynchronizationContext.Current) return true;
                var memberInfo = SynchronizationContext.Current?.GetType();
                if (memberInfo != null && memberInfo.Name.EndsWith("SingleThreadSynchronizationContext")) return true;

                return false;
            }
        }
```
Now the application starts again, although in a rather smelly fashion using a string comparison on a type name.
comment
created
Cheesebaron
username_0
@username_5 thanks for trying this code out. I will have a go at fixing this iOS issue.
comment
created
Cheesebaron
username_0
OK. I've tested on UWP, WPF, Android and iOS now. All seem to work when checking whether Current SynchronizationContext is SingleThreadedSynchronizationContext.

Since we are the only ones being able to create it and we always do that from the UI thread, it should be safe to do.
comment
created
flostr
username_6
Please fix this. I did spent half a day finding the cause of this issue. For me this happens exactly like described in [#3209](https://github.com/MvvmCross/MvvmCross/issues/3209).

I currently fixed this by removing the **await** in **MvxAppStart<TViewModel>.NavigateToFirstViewModel**>
```
        protected override async Task NavigateToFirstViewModel(object hint = null)
        {
            try
            {
                //await NavigationService.Navigate<TViewModel>();
                NavigationService.Navigate<TViewModel>();
            }
            catch (System.Exception exception)
            {
                throw exception.MvxWrap("Problem navigating to ViewModel {0}", typeof(TViewModel).Name);
            }
        }
```
comment
created
nickrandolph
username_1
Hey all, what do we need to get this merged?
comment
created
Cheesebaron
username_0
Android sync does still not work. I'll try to find some time to fix it. But if someone wants to chip in they are welcome to do that.

Similarly to the other platforms, we need to pump the tasks that are run during startup of MvvmCross, this needs to happen on Android as well.
comment
created
zevsst
username_7
Observing this issue since our projects migrated to MvvmCross 6.2.3 in December 2018. Spent 1 day to figure out what was going wrong.

The temp solution is:
`public class CustomMvxAppStart : MvxAppStart
	{
		public CustomMvxAppStart(IMvxApplication application, IMvxNavigationService navigationService) : base(application, navigationService)
		{
		}

		protected override async Task NavigateToFirstViewModel(object hint = null)
		{
			// Never do this until Cross team gets this issue fixed
			// await NavigationService.Navigate<LoginViewModel>();

			// https://github.com/MvvmCross/MvvmCross/pull/3222
			// https://github.com/MvvmCross/MvvmCross/issues/3209 - warkaround was taken from here
			// https://github.com/MvvmCross/MvvmCross/issues/2953
			// https://github.com/MvvmCross/MvvmCross/issues/3221

            NavigationService.Navigate<SplashViewModel>();
		}
	}`

Are you going to provide any fix?
comment
created
flostr
username_6
This PR looks like a proper solution by now. When can we expect it to be merged?
comment
created
arek-avanade
username_8
Instead of introducing custom sync context or any other complex solutions, why not just providing a different base view model class for the initial view, which doesn't have ANY async init methods, just sync? This could be the only class that MvxAppStart accepts. 
Each mobile developer would have to consciously make a decision how he/she wants to handle app start: it can be either purely synchronous OR one could use Task.Run(...).Wait() in his sync init method to run any asynchronous code and wait for the result on the main thread. This puts more control to the app developer and would let MvvmCross team avoid dirty solutions like `.GetAwaiter().GetResult()`.
comment
created
nickrandolph
username_1
What you've proposed is my option 1 in my response over at https://github.com/MvvmCross/MvvmCross/issues/3395 

Unfortunately simply having a different base class isn't the right answer:
- It's not adequate to make it synchronous because there are some thing you may want to do that are async actions. For example just doing a file read on UWP is an async operation. Simply creating a synchronous option doesn't help.
- Duplicating the base class means that we need to have specific logic for the first navigation, which means more code to maintain and update as we evolve the platform.

Assuming we can get @username_0 's proposed solution to work (which I think it pretty close now) this should be transparent to most devs and will allow them to put async actions in the Initialize method whilst still allowing all platforms to start up correctly.
comment
created
arek-avanade
username_8
I understand the need for async calls during start up, hence there is always possibility to use `Task.Run(...).Wait()` even in a sync method.
I don't know the whole MvvmCross codebase, but if it's hard to introduce a different base view model class for start up, then I think the problem is not with the sync start up but with the current framework design. 
AsyncPump might be an elegant solution, but requires very specific knowledge to understand it. If there is a bug in this code, there are probably just a few people here who can fix it (or even just one - the author :) ), and also debugging the start up sequence in applications might be difficult.
If you guys want to push this forward as an interim solution, that makes sense to me, but I would not stick to that approach longer than absolutely needed.
comment
created
FinHorsley
username_9
@username_1 Do we really need async to cater for UWP? Hopefully i haven't missed a howler, but frameworks like Xamarin.Forms just have a synchronous startup, `override void OnStart()`

The startup here is an inherently synchronous process, so shouldn't this be reflected in the framework? 🙂
comment
created
DavidMarquezF
username_10
This is currently causing problems in our app. Are there any plans to merge this??
comment
created
yanxiaodi
username_11
Hi, guys, any updates for this issue? Thanks.
comment
created
Cheesebaron
username_0
@username_11 no. Do you want to take over and finish it up?
comment
created
yanxiaodi
username_11
@username_0 I've tried but the solution crashes frequently on my laptop. I'll investigate it later.
comment
created
yanxiaodi
username_11
@username_0 @username_1  so now what we need to do for this PR? I think the key thing here is to make sure that the app can start properly even if the lib user uses an async method in the AppStart.
comment