Calling async methods from main thread is a bad idea?

I've just read @JGoldberger blogpost Getting Started with Async / Await and I think it has a hidden pitfall.

It says, that if we call await DoSomething();, it will be compiled to the same code as Task.Factory.StartNew(() => { DoSomething(); }), which performs the DoSomething method on a new (background) thread, but in fact, if I call await DoSomething(); from the main thread and it contains some CPU-heavy synchronous computing inside (like this), it will actually be executed from the main thread and will block UI.

Usually, we don't do any CPU-heavy synchronous methods, but that was just an example. The problem is, no matter what I call inside the DoSomething method, it still gets executed on the main thread and slowing down the app, right?! So we should generally avoid calling await DoSomething(); from the main thread, like from protected override async void OnResume() on Android and instead ALWAYS call:
await Task.Run(async () => await DoSomething());

This seems like something that developers are probably not aware of - or at least I wasn't. Calling methods with await on main thread is something that I have always considered a good practice.
Please, tell me that I'm wrong :hushed:

Tagged:

Posts

  • ClintStLaurentClintStLaurent USUniversity ✭✭✭✭✭

    @DavidRiha said:
    but in fact, if I call await DoSomething(); from the main thread and it contains some CPU-heavy synchronous computing inside (like this), it will actually be executed from the main thread and will block UI.

    Interesting. Since everything hereafter is based on that premise... Where is that statment coming from? What makes you feel that it takes place on the UI thread? And where is the cut-off for "some CPU-heavy computing"

  • DavidRihaDavidRiha USMember ✭✭
    edited September 2018

    @ClintStLaurent Simple - I tried it. I called an async method with await inside OnResume, and on first line of that async method, called the method FindPrimeNumber(100000) from the mentioned stackoverflow link to test that it really does block the UI. In real life scenario, we will usually not dothings like finding prime numbers, but every millisecond is precious, when blocking the UI thread.

  • ClintStLaurentClintStLaurent USUniversity ✭✭✭✭✭

    Interesting. Well... I have some free time today. And my testing app open. I think I'll try pulling that same testing methodology into it and bang on it. If it really is the case, it would be of benefit to confirm it so I don't think the boss can be mad for me testing.

  • ClintStLaurentClintStLaurent USUniversity ✭✭✭✭✭

    @DavidRiha said:
    I called an async method with await inside OnResume,

    So how did you do that since OnResume is not async. Generally you get an error for trying that.

    and on first line of that async method, called the method FindPrimeNumber(100000) from the mentioned stackoverflow link

    Ok. But that method as presented in S.O. is not an async method. So did you convert them to be asynchronous? That's what I'm doing now.

  • DavidRihaDavidRiha USMember ✭✭

    Just read again what I wrote, especially "protected override async void OnResume()" and "and it contains some CPU-heavy synchronous computing inside".

    I am talking about the fact that synchronous code is performed on the main thread, which I think is bad.

  • DavidRihaDavidRiha USMember ✭✭
    edited September 2018
    protected override async void OnResume()
    {
        //show preloader
    
        var result = await SlowAsync(); //blocks UI
        //---versus---
        var task = SlowAsync();
        var result = await task; //also blocks UI
        //---versus---
        var result = await Task.Run(async ()=> await SlowAsync()); //does not block UI
    
        //update UI, hide preloader
    }
    
    public async Task<bool> SlowAsync()
    {
        FindPrimeNumber(100000); //or whatever sync, usually not that CPU-heavy, but still blocking the UI for some precious milliseconds (some conditions, sorting some lists, anything)
    
        await Task.Delay(10); //or whatever async, some server fetch, this is not blocking the UI, the main thread is released until it is finished .. unless it also contains some synchronous code
    
        return true;
    }
    
  • ClintStLaurentClintStLaurent USUniversity ✭✭✭✭✭

    @DavidRiha said:
    I am talking about the fact that synchronous code is performed on the main thread, which I think is bad.

    Oh. Yeah. Synchronous code on the main thread is bad. I don't think anyone thinks otherwise.
    And the methods you said you are testing with are not asynchronous. So I would expect them to hold up.

    So... I'm lost. What part of any of that is a surprise or nothing working the way you think it should?

  • DavidRihaDavidRiha USMember ✭✭

    @ClintStLaurent see the sample, I've just posted a second before you :)

  • ClintStLaurentClintStLaurent USUniversity ✭✭✭✭✭
    edited September 2018

    protected override async void OnResume()

    You're try to change the signature and very nature of a method you're overriding. I'm not sure that even has any affect on the base synchronous method or the behavior of the ovefride. I don't think you can just magically make the synchronous base behavior suddenly async like that. For example you can't override a public base with a private override: Its not legal. Which might explain why you are still seeing synchronous behavior.

    But since your second effort in there is making a true new task that really is asynchronous then it doesn't block the UI. That seems right to me.

  • DavidRihaDavidRiha USMember ✭✭
    edited September 2018

    No, I don't think there is anything wrong with using a virtual void method as override async void. And most importantly, it doesn't matter, it was just an example. You would most probably get the very same behavior if the method was not OnResume but any other async method running on main thread.

    What I am trying to say is, that we should probably NEVER call await DoSomething() from ANY method that is running on the main thread (unless we need the stuff inside DoSomething() also being performed on the main thread, which is usually not the case). For the best performance and UI responsiveness, we should ALWAYS call await Task.Run(async () => await DoSomething()); and that is a very surprising fact for me.

  • JGoldbergerJGoldberger USMember, Forum Administrator, Xamarin Team, University Xamurai
    edited September 2018

    @DavidRiha,

    I am basically correct in what I said in my blog post. DoSomething does have to be asynchronous and does have to either launch a new thread with Task.StartNew or Task.Run (or one of many other methods of starting threads) or call another async method that launches a new thread. Yes, someone has to launch a new thread for a method to be async, whether the method itself or a method it calls. That is the end of that story.

    And this method you showed is basically synchronous (and by the way all code that runs on the UI thread is synchronous... every change to a text or color or view is synchronous code happening on the UI thread)

    public async Task<bool> SlowAsync()
    {
        FindPrimeNumber(100000); //or whatever sync, usually not that CPU-heavy, but still blocking the UI for some precious milliseconds (some conditions, sorting some lists, anything)
    
        await Task.Delay(10); //or whatever async, some server fetch, this is not blocking the UI, the main thread is released until it is finished .. unless it also contains some synchronous code
    
        return true;
    }
    

    The part that is taking up the time, FindPrimeNumber is indeed running on the UI Thread as you have not launched a thread to run that code on. So you block. Sure, when the code hits the await task.Delay call, control will return to the calling thread and method.... but that method won't finish yet as it is awaiting the result, which will come after the delay. And any code in OnResume that comes after the await won't run until the async method completes. Exactly as happens with:

    Task.Run(()=>{...async code ... }).ContinueWith(()=> {... code running synchronously back on the main thread }, TaskScheduler.FromCurrentSynchronizationContext ());
    

    So what you should do in SlowAsync is:

    public async Task<bool> SlowAsync()
    {
        await Task.Run(() => {
            FindPrimeNumber(100000); //or whatever sync, usually not that CPU-heavy, but still blocking the UI for some precious milliseconds (some conditions, sorting some lists, anything)
        });
        return true;
    }
    

    (not tested, but should be good... though why you are returning bool I have no idea, I would think you want FindPrimeNumber to return a prime number, i.e. an int or a long more likely and then there is no reason not to make that the async method as that is the long running method. That is the method that should need to be awaited and that should start a thread to do its work.

    Anyway, Looking back at the blog post, I could have been a bit clearer that in the case of await DoSomething() that DoSomething would have to eventually launch a new thread itself or call a method that does, otherwise it is synchronous.

  • DavidRihaDavidRiha USMember ✭✭
    edited September 2018

    Thank you for the thorough explanation. So yeah, I actually am doing things wrong.

    For example, in my app I have a code like this (a lot of code like this):

    protected override async void OnResume()
    {
            base.OnResume();
    
            if (ParentComment == null)
            {
                // show preloader
    
                ParentComment = await GetCommentsForRepliesScreen(CommentId);
             }
    
             // refresh UI
    }
    
    private async Task<CommentEntity> GetCommentsForRepliesScreen(string commentId)
    {
            var dict = await Extensions.ParseExtensions.CallFunctionSafelyAsync<object>("getCommentData", new Dictionary<string, object>() { { "commentId", commentId } }, false)
                 as Dictionary<string, object>;
    
            var currentUser = await LoginViewModel.GetCurrentUserAsync();
    
            //---------- I never realized that all of this is running on UI thread and making the app sluggish  ------
            if (dict == null || !dict.ContainsKey("parent"))
                return null;
    
            var parentDict = dict["parent"] as Dictionary<string, object>;
    
            if (parentDict == null || !parentDict.ContainsKey("entity") || !parentDict.ContainsKey("liked"))
                return null;
    
            var commentObj = parentDict["entity"] as ParseObject;
    
            if (commentObj == null)
                return null;
    
            var comment = new CommentEntity(commentObj) { CurrentUserLikesThis = (bool)parentDict["liked"] };
    
            //------------ still running on UI thread, and this method is actually quite complicated, creating objects, ordering lists, about 100 lines of cude --------
            comment.ProcessCommentsResult(dict, currentUser);
    
            return comment;
    }
    

    So, calling ParentComment = await Task.Run(()=>GetCommentsForRepliesScreen(CommentId)); or enclosing the code inside GetCommentsForRepliesScreen() in await Task.Run is the way to go.

    I guess I will never stop learning :blush:

    Thank you.

  • JohnHardmanJohnHardman GBUniversity mod
    edited October 2018

    @DavidRiha said:
    So, calling ParentComment = await Task.Run(()=>GetCommentsForRepliesScreen(CommentId)); or enclosing the code inside GetCommentsForRepliesScreen() in await Task.Run is the way to go.

    However you move CPU intensive code off of the UI thread, one thing you don't want to do is to call async methods from the lifecycle methods (OnStart, OnResume, OnSleep). Doing so will result in unexpected and (often) hard to reproduce behavior. Why? Because those methods return void and not Task or Task<t>. That means that whatever code calls those methods is not going to await them, which is very likely to result in a race condition. That's the case even if you await the async methods that you call from within the lifecycle methods.

  • JGoldbergerJGoldberger USMember, Forum Administrator, Xamarin Team, University Xamurai
    edited October 2018

    Lifecycle methods are similar to event handlers... both are called by external code which we have no control over. We know it is OK to call an async method from an event handler (which always return void), so I see no reason why you can't call an async method from a lifecycle method that returns void. You will need to make sure you handle any exceptions thrown from the async method call.

    Of course you cannot call an async method from a method that is called by external code if the method returns a type, T, since you cannot change that method to return Task<T> as the calling method cannot handle getting a Task back (i.e. as you said it is not awaited), but you can use Task.Run(async () => { await SomeMethodAsync});

  • JohnHardmanJohnHardman GBUniversity mod
    edited October 2018

    @JGoldberger said:
    I see no reason why you can't call an async method from a lifecycle method that returns void.

    A couple of examples:

    (1) If you override App.OnSleep and put the following in an async override:

                    await Task.Delay(20000);
                    System.Diagnostics.Debug.WriteLine("After 20");
    

    Does the WriteLine get executed? It depends on how quickly the app is destroyed (change the delay to 20 and the WriteLine probably will get executed). Would you write that code in practice? No, but it demonstrates the problem, particularly if you have a slow async task or if you have a number of async tasks. You might want to create a scheduled local notification (which using Allan Ritchie's current version of his notification plugin is an async method), and/or you might want to write to a DB, and/or to a WebServer, and/or use Application.SavePropertiesAsync - all of which could have problems.

    (2) Possibly more surprising, what happens if you use await in an override of App.OnStart? Most of the time, it will seem to work. However, UWP demonstrates a race condition. When I first used James Montemagno's plugins, I wanted to start picking up location information as quickly as possible, so initialised the plugin from OnStart. That involves using await. The plugin could pop up a message box asking "Let appname access your precise location?". Once that message box was closed and the app used in earnest, the race condition results in the navigation back button sometimes not appearing. I reproduced that same result without using the plugin, just using a simple awaited Task.

    (3)

    @JGoldberger said:
    you can use Task.Run(async () => { await SomeMethodAsync});

    If you move the code from (1) into SomeMethodAsync, whether the WriteLine gets executed still depends on how quickly the app is destroyed.

    @JGoldberger said:
    Lifecycle methods are similar to event handlers... both are called by external code which we have no control over. We know it is OK to call an async method from an event handler (which always return void), so I see no reason why you can't call an async method from a lifecycle method that returns void

    TBH, event handlers (such as handling a Button click) can suffer from similar problems. This is why if you have a Button click handler that does await PushAsync you'll probably want to put a check in that the same page doesn't get pushed twice if the user does a second click/tap before the PushAsync has completed.

    The difference between event handlers and lifecycle methods is the seriousness of what happens when the wind is blowing the wrong direction. With an event handler, you'll hopefully have other code that prevents serious problems. With the lifecycle methods, your data might not get persisted, a notification might be lost etc.

  • JGoldbergerJGoldberger USMember, Forum Administrator, Xamarin Team, University Xamurai
    edited October 2018

    @JohnHardman

    Those are good points, but as you point out the same issues can occur in button click event handlers where it is accepted practice to use async and await. IOW there is nothing to stop an app from being closed by the user after tapping a button that launches an async method, so your sample code in OnSleep can have the same result if used in a button click event handler if the user closes the app before the async method completes. So all you have really pointed out is that whenever you call an async method you need to check for possible race conditions. This is always true no matter where you call an async method from. In the examples you give, one could always use a cancellation token to cancel an async task when the app is exiting or a page is exiting.

    But yes, calling an async method from OnSleep is likely not a good idea as it is expected that the app will be closing. If you need to save data or do some other important things when the app is closing/going to sleep, then you should start a background task as per the OS recommendations (iOS / Android ).

    Bottom line, yes potential race conditions have to be guarded against no matter where you call an async method from. I stand by my assertion that using async and await from lifecycle methods that return void is fine... as long as you guard against any potential race conditions, which always has to been done. Following your logic above, one should never call async methods from anywhere and clearly that is not true. No matter what API you use, working with multi threaded applications adds complexity that requires attention on the part of the developer to guard against conditions such as those you describe.

  • JohnHardmanJohnHardman GBUniversity mod
    edited October 2018

    @JGoldberger said:
    IOW there is nothing to stop an app from being closed by the user after tapping a button that launches an async method

    Agreed.

    @JGoldberger said:
    In the examples you give, one could always use a cancellation token to cancel an async task when the app is exiting

    One could, but doing so doesn't accomplish anything useful in the scenarios mentioned for lifecycle methods. In those scenarios the tasks want to complete, not be cancelled.

    @JGoldberger said:
    I stand by my assertion that using async and await from lifecycle methods that return void is fine... as long as you guard against any potential race conditions, which always has to been done.

    I think we'll have to disagree on that one. The app developer may only have control over one of the two (or more) paths of execution, so cannot guard against some race conditions if using await in lifecycle methods. In the case of the back navigation button sometimes not appearing if await is used in OnStart on UWP, the developer would have no clue (I certainly haven't) as to how await could be used without the race condition existing. I'd have to delve deep in the XF source code to find the answer to that one.

    @JGoldberger said:
    Following your logic above, one should never call async methods from anywhere

    Not at all. Depending on the nature of the async work being done, different async methods need to be handled differently. Only in very specific situations should async methods be avoided.

    @JGoldberger said:
    No matter what API you use, working with multi threaded applications adds complexity that requires attention on the part of the developer to guard against conditions such as those you describe.

    Agreed.

    @JGoldberger said:
    If you need to save data or do some other important things when the app is closing/going to sleep, then you should start a background task as per the OS recommendations

    Agreed.

    However, I don't recall ever seeing a Xamarin.Forms sample that does this in the scenario where one would want to (but cannot) safely make a number of async calls in OnSleep before the app gets destroyed. Do you know of a sample that does this for a XF app targeting all of Android/iOS/UWP?

    TBH, it shouldn't be that hard to do (I'd just want to check whether MessagingCenter is guaranteed to get a message safely to the background task before the app is destroyed), but it would be a good one to include as one of the Xamarin samples, if only to make people aware of the benefits of the approach.

    BTW, whilst writing this, I had a quick look at the documentation of SavePropertiesAsync and the current XF source code.
    The documentation is at https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.application.savepropertiesasync?view=xamarin-forms
    It says: "The developers uses this method to perist the application state that is currently stored in Properties immediately, rather than waiting for a life cycle event to trigger the data to be saved to the device's permanent storage."
    That is a bit misleading - "a life cycle event" is non-specific, so suggests that all life cycle events save the data. That's not the case. The source code shows that the save only happens after "OnSleep" returns. It might be worth updating the documentation to make that clearer (as well as changing "perist" to "persist", and "The developers uses" to "The developer uses"). It's also a concern that on Android and iOS the code uses a method commented with "Don't use this unless there really is no better option" and that calls a method that is the dreaded "async void". Those who remember the history of bugs around SavePropertiesAsync will be wary.

    Of course, having read that documentation and source code does mean one change to my earlier post - although one could call Application.SavePropertiesAsync from OnSleep (although still with the issues mentioned), there is actually no need to, so that one can be ignored.

  • JGoldbergerJGoldberger USMember, Forum Administrator, Xamarin Team, University Xamurai
    edited October 2018

    @JohnHardman said:

    @JGoldberger said:
    In the examples you give, one could always use a cancellation token to cancel an async task when the app is exiting

    One could, but doing so doesn't accomplish anything useful in the scenarios mentioned for lifecycle methods. In those scenarios the tasks want to complete, not be cancelled.

    Of course, so if you have a task that MUST complete, it is up to you to make sure it CAN complete in the time allotted. As mentioned, you may need to use the iOS backgrounding methods to make sure that those tasks have time to complete.

    @JGoldberger said:
    I stand by my assertion that using async and await from lifecycle methods that return void is fine... as long as you guard against any potential race conditions, which always has to been done.

    I think we'll have to disagree on that one. The app developer may only have control over one of the two (or more) paths of execution, so cannot guard against some race conditions if using await in lifecycle methods. In the case of the back navigation button sometimes not appearing if await is used in OnStart on UWP, the developer would have no clue (I certainly haven't) as to how await could be used without the race condition existing. I'd have to delve deep in the XF source code to find the answer to that one.

    I will have to look into this particular scenario, but it sounds like one specific scenario and thus it does not seem like it is a good basis for a general statement like "async methods should not be called from lifecycle methods." Instead it is only the basis for the statement "Be careful when, or avoid, calling async methods from the UWP OnStart lifecycle method."

    So, unless I compelled you to change your mind with the above rebuttal, yes, we will have to agree to disagree. :-)

    @JGoldberger said:
    Following your logic above, one should never call async methods from anywhere

    Not at all. Depending on the nature of the async work being done, different async methods need to be handled differently. Only in very specific situations should async methods be avoided.

    Exactly. And "lifecycle methods" do not seem to be one of those situations to me (as long as they return void and any potential race conditions are guarded against).

    However, your logic was "don't call an async method from OnSleep because the app will be closing and the async method may not complete, or from a button click event handler because the user may close the app, and thus the async method m ay not complete" then all bets are off and all places are of limits to async method calls. But that is not the case, so again I say that a developer must guard against race conditions no matter from where they call an async method. And perhaps again we must agree to disagree.

    @JGoldberger said:
    If you need to save data or do some other important things when the app is closing/going to sleep, then you should start a background task as per the OS recommendations

    Agreed.

    However, I don't recall ever seeing a Xamarin.Forms sample that does this in the scenario where one would want to (but cannot) safely make a number of async calls in OnSleep before the app gets destroyed. Do you know of a sample that does this for a XF app targeting all of Android/iOS/UWP?

    Not off hand, but samples are usually limited to demonstrating specific functions and mostly don't include the need to save complex application state in OnSleep.

    TBH, it shouldn't be that hard to do (I'd just want to check whether MessagingCenter is guaranteed to get a message safely to the background task before the app is destroyed), but it would be a good one to include as one of the Xamarin samples, if only to make people aware of the benefits of the approach.
    BTW, whilst writing this, I had a quick look at the documentation of SavePropertiesAsync and the current XF source code.
    The documentation is at https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.application.savepropertiesasync?view=xamarin-forms
    It says: "The developers uses this method to perist the application state that is currently stored in Properties immediately, rather than waiting for a life cycle event to trigger the data to be saved to the device's permanent storage."
    That is a bit misleading - "a life cycle event" is non-specific, so suggests that all life cycle events save the data. That's not the case. The source code shows that the save only happens after "OnSleep" returns. It might be worth updating the documentation to make that clearer (as well as changing "perist" to "persist", and "The developers uses" to "The developer uses").

    I fixed these typos in master branch of the docs repository.

    I'll need to do a bit of investigation with Application.SavePropertiesAsync to verify the behavior you described. If it is not behaving as expected, please feel free to file an issue in the Xamarin.Forms github repository with a test project that demonstrates the issue. Also have you tested the behavior with the latest version of Forms? Last I checked you were still using Xam.Forms ~2.5.

  • JohnHardmanJohnHardman GBUniversity mod
    edited October 2018

    @JGoldberger said:
    Last I checked you were still using Xam.Forms ~2.5.

    Now on XF 3.2.0.871581, although still re-coding where plugins updated at the same time have introduced breaking changes, and will still need to remove from my code stuff that I implemented a long time ago that has since been added to XF 3.x

    [Edit] Downgraded to 3.2.0.839982 as 3.2.0.871581 caused build problems on UWP (Android and iOS were fine).

  • AllanRitchieAllanRitchie CAInsider, University ✭✭✭

    I heard my name and thought I would give my two cents. Normally, I would say to block on your async method using .Wait() on app sleep. OnResume, fire and forget obviously works in most scenarios.

    For app sleep obviously it is a bit harder as noted, you can always .Wait it as long as there isn't anything else going on, it shouldn't deadlock. From Android perspective, it normally won't shutdown a background task right away in any case even with doze. You can always have a perma service running to keep your app alive in any case. iOS is usually the immediate problem, however they do give you a mechanism of spawning a background job as your app is about to sleep. UIApplication.SharedApplication.BeginBackgroundTask/EndBackgroundTask.

    You can also try out my background jobs framework using RunTask (https://github.com/aritchie/jobs) which does the above in an xplat way if interested :)

  • DanielMezaDanielMeza COMember ✭✭
    edited July 29

    The whole error here is the fact that the compiler does not generate a Task.Factory.StartNew when we compile our app, in fact its create a state machine to handle it like a TaskCompletionSource strategy, you can see the compiler generated code when you call an async method In this post here .

    To clarify we need to remember that there are to kind of applications model Single Thread (Client apps) like WPF, Xamarin, and Multy Thread (Server apps) like WCF, ASP.NET Web apps. Those model impact how app handle our task. When we say Task.Factory.StartNew or if abbreviated and recommended method Task.Run we leverage the Task execution to the app model strategy.

    So when we only call await SomeAsync(); this is running async but this task is running by the Main Thread so it will block the execution when is doing to much work. Thas why if we need to definitely not to block the UI we need run the task in another Thread by calling Task.Run, remenber not to modify any UI object in a background task and sync the UI context, it is safe to await a Task.Run and then modify the UI because the code write before to await it will be running in the UI Thread, so you can send any code that dos't need UI interaction like calls to DB, make some HTTP request and read a file like

    async Task GetDataAsync()
    {
        var result = await Task.Run(_repository.GetDataAsync);
        countTextBox.Text = result.Count();
    }
    

    some code are async so you don't need create a Task.Run because it does in the inside.

Sign In or Register to comment.