Can I call an async method from OnOptionsItemSelected?

Question

I'm teaching myself to use the Xamarin Platform (I'm also new to C#). I'm creating an app where the user logs on and is forced to create a profile. I'd like to have the ActionBar (I'm using the material toolbar) contain a menu item DONE. When the user clicks DONE, my app verifies the input data and sends the data to my Parse backend. The problem with this is that the Parse API requires an await profile.SaveAsync(); in order to do this. In order to determine that the user clicked DONE in the ActionBar, I need to override OnOptionsItemSelected(IMenuItem item), which is not asynchronous.

I've found a way around this by creating private async void ActionDone() that will handle all the Parse connections. I then call ActionDone() within my switch statement in OnOptionsItemSelected. However, I think this is tying up the UI thread with await. I've read that this is a bad idea (mostly on other StackOverflow posts). Is there another way to get the ActionBar to await? Or am I safe because in order to proceed, the Profile needs to be saved and therefore holding up the UI is "acceptable"?

OnOptionsItemSelected

public override bool OnOptionsItemSelected(IMenuItem item)
    {
        // verify nothing else was clicked
        switch(item.ItemId)
        {
            case Resource.Id.action_done:
                ActionDone();
                break;
            default:
                // log error message
                break;
        }

        return base.OnOptionsItemSelected(item);
    }

ActionDone

private async void ActionDone()
    {
        Task<ApiHandler.CreateProfileStruct> createProfileTask = ApiHandler.CreateProfile(mNameEdit.Text, mPhoneEdit.Text, mEmailEdit.Text, mSeriesEdit.Text, mLocationEdit.Text);
        var result = await createProfileTask;

        // toast the result...
        Toast.MakeText(this, result.message, ToastLength.Long).Show();

        // check if profile was created
        if (result.enumValue == ApiHandler.CreateProfileEnum.Success)
        {
            StartActivity(typeof(MainActivity));
            Finish();
        }
    }

All my parse calls are in a shared library so I'll be able to use them with iOS as well

public static async Task<CreateProfileStruct> CreateProfile(string name, string phone, string email, string series, string location)
    {
        Profile profile = new Profile(name, phone, email, series, location);

        CreateProfileStruct result = new CreateProfileStruct();
        string validation = profile.validate();

        // profile information is acceptable...
        if (validation.Equals(""))
        {
            Console.WriteLine("creating profile");

            try
            {
                await profile.SaveAsync();
            }
            catch (ParseException e)
            {
                // set enum to error
                result.enumValue = CreateProfileEnum.Error;

                // determine the error message
                if (e.Code == ParseException.ErrorCode.ConnectionFailed)
                    result.message = parseNoConnection;
                else
                    result.message = profileCreationFailed;

                // return
                return result;
            }

            result.enumValue = CreateProfileEnum.Success;
            result.message = profileCreated;

            // change ParseUser["hasProfile"] to true
            ParseUser user = ParseUser.CurrentUser;
            user["hasProfile"] = true;
            user.SaveAsync();

            return result;
        }
        // profile info is not acceptable
        else
        {
            result.enumValue = CreateProfileEnum.Error;
            result.message = validation;
            return result;
        }
    }

    public enum CreateProfileEnum
    {
        Success,
        Error
    }

    public struct CreateProfileStruct
    {
        public CreateProfileEnum enumValue;
        public string message;
    }

I should add that I've already implemented the code this way, and it works (as far as I can tell). Just based on what I've read, I'm thinking it's not the best strategy.

Answer 1

In response to your comment:

do you mean that the return in OnOptionsItemSelected() has the possibility to execute before the await finishes

Yes. In fact, that is the likely outcome (i.e. you intend for the operation to be asynchronous, so the typical case is for the operation to complete asynchronously).

Or am I safe because in order to proceed, the Profile needs to be saved and therefore holding up the UI is "acceptable"?

I would not say that blocking the UI thread is ever acceptable. Granted, it can be simpler to implement: whenever you have asynchronous operations while the UI thread is free to execute, then you have to worry about the state of the UI, whether the user can click on or otherwise issue commands that may or may not be valid while the asynchronous operation is in progress, etc.

But frankly, that's exactly the kind of issues that the async/await feature is designed to make a lot easier. You can write code in a linear way that reconfigures the UI for the duration of an asynchronous operation (if needed) and then just as easily put things back when it's done.

As your code is written now, the asynchronous operation will not block the UI thread. But the OnOptionsItemSelected() method does return, first calling the base implementation, before that operation has completed.

Whether this is a problem in your case, I don't know. There's not enough context here. But…

The only other action in the method is to call the base implementation and return that implementation's result. As long as there is nothing in that base implementation that would depend on the outcome of the asynchronous operation, and as long as returning the base implementation's return value from the method before the asynchronous operation has completed doesn't mislead the caller (and if the base implementation doesn't depend on the asynchronous operation, I would think it wouldn't), it should be fine.

If the base implementation does depend on the outcome of the asynchronous operation, you can wrap the whole method body in an async method so that you can await the asynchronous operation and defer calling of the base implementation until the operation has completed. For example:

public override bool OnOptionsItemSelected(IMenuItem item)
{
    var ignoreTask = OnOptionsItemSelectedAsync(item);

    return true;
}

private async Task OnOptionsItemSelectedAsync(IMenuItem item)
{
    try
    {
        // verify nothing else was clicked
        switch(item.ItemId)
        {
            case Resource.Id.action_done:
                await ActionDone();
                break;
            default:
                // log error message
                break;
        }

        bool result = base.OnOptionsItemSelected(item);

        // If some action should be taken depending on "result",
        // that can go here
    }
    catch (Exception e)
    {
        // The caller is ignoring the returned Task, so you definitely want
        // to observe exceptions here. If you have known exceptions that can
        // be handled reasonably, add an appropriate "catch" clause for that
        // exception type. For any other exceptions, just report/log them as
        // appropriate for your program, and rethrow. Ultimately you'd like
        // that to cause the entire process to crash with an "unobserved task
        // exception" error, which is what you want for an exception you didn't
        // anticipate and had no way to actually handle gracefully. Note that
        // in .NET 4.5 and later, unobserved exceptions WILL NOT crash the process,
        // unless you set the ThrowUnobservedTaskExceptions option to "true"
        // in your App.config file. IMHO, doing so is a VERY good idea.

        throw;
    }
}

// This has to be awaitable...you should eschew "async void"
// methods anyway, so this is a change you'd want to make regardless.
private async Task ActionDone() { ... }

But you'll note that in the above, the actual overridden method still has to return some value. In other words, you wind up having to lie to the caller, and (optionally) having to deal with the actual outcome of the base implementation later.