Callback problems with WPF

Question

I've been running into a Callback problems with async programming in WPF .Net 4.5. There should be a way of doing this code in a more understandable way (I have suppressed a big part of it to make it easier to see what is going on).

I don't think there is a way to simply remove code because I need to call Dispatcher in order to manipulate WPF controls and calls like in TbSequence.Focus() and Utils.ShowMessageBox.

private void Save_Executed(object sender, ExecutedRoutedEventArgs e)
{
    try
    {
        Controller.Busy = true;

        System.Threading.Tasks.Task.Run(() =>
        {
            try
            {
                Controller.SaveItem();
            }
            catch (BdlDbException ex)
            {
                if (ex.ExceptionSubType == DbExceptionSubTypes.UniqueViolation)
                {
                    HandleUniqueViolation(ex);
                }
                else
                {
                    string errorMessage = "";
                    errorMessage = ex.Message;
                    Dispatcher.BeginInvoke(new Action(() => Utils.ShowMessageBox(t_MessageBox.Attention, errorMessage)));
                }
            }

            Controller.Busy = false;
        });
    }
    catch (FieldException ex)
    {
        if (ex.FieldName == "FirstName")
        {
            TbFirstName.ValidationError = true;
            TbFirstName.ApplyErrorToolTip(ex.Message);
        }
    }
}

public void Init(UcEdit container, Employee entity = null)
{
    Controller.Busy = true;

    System.Threading.Tasks.Task.Run(() =>
    {
        try
        {
            Controller.Init(entity);
        }
        catch (BdlEntryNotFoundException ex)
        {
            HandleNotFoundException(ex);
        }

        Container.Dispatcher.BeginInvoke(new Action(() =>
        {
            Container.DataContext = Controller;

            // Instructions order for focusing TbSequence after load should be different in case we have an existent item
            // because we have to focus the control AFTER it is filled with data, in order to set the caret position correctly.
            if (Controller.IsNewItem)
            {
                this.DataContext = Controller;
                TbSequence.Focus();
                Controller.Busy = false;
            }
            else
            {
                TbSequence.TextChanged += TbSequence_TextChanged;
                this.DataContext = Controller;
                SetButtons();
            }
        }));
    });
}

private void HandleUniqueViolation(BdlDbException ex)
{
    string errorMessage = "";
    bool isSequence = false; // if true, it's name.

    if (ex.Fields[1] == "Sequence")
    {
        errorMessage = "There is already an Employee with the sequence \"" + Controller.Item.Sequence + "\".";
        isSequence = true;
    }
    else
    {
        errorMessage = "There is already an Employee named \"" + Controller.Item.FirstName +
        " " + Controller.Item.LastName + "\".";
    }

    errorMessage += "\r\nLoad it from Database?\r\n(All the changes on this window will be lost.)";

    Dispatcher.BeginInvoke(new Action(() =>
    {
        MessageBoxResult res = Utils.ShowMessageBox(t_MessageBox.Question, errorMessage, MessageBoxButton.YesNo);

        switch (res)
        {
            case MessageBoxResult.Yes:
                if (isSequence)
                {
                    System.Threading.Tasks.Task.Run(() =>
                        {
                            Controller.GetEmployeeBySequence(Controller.Item.Sequence);
                            Init(Container, Controller.OriginalItem);
                        });
                }
                else
                {
                    System.Threading.Tasks.Task.Run(() =>
                        {
                            Controller.GetEmployeeByName(Controller.Item.FirstName, Controller.Item.LastName);
                            Init(Container, Controller.OriginalItem);
                        });
                }
                break;

            case MessageBoxResult.No:
                break;
        }
    }));
}

As you can see, there is a major Callback problem here that behaves like this:

Save_Executed (UI) -> HandleUniqueViolation (Task) -> ShowMessageBox (UI) -> Controller.GetEmployeeBySequence (Task) -> Controller.Init ...

And so it goes. Is there a way to make this code more easy to read? Thank you.

Answer 1

You're starting off your code by putting (more or less) the entirety of your method body in a call to Task.Run and then explicitly marshalling to the UI thread all over the place within that callback.

Just narrow the scope of your Task.Run call so that your UI code is just outside the call to Run rather than inside both it and a call to Invoke:

private async void Save_Executed(object sender, ExecutedRoutedEventArgs e)
{
    try
    {
        Controller.Busy = true;

            try
            {
                await Task.Run(() => Controller.SaveItem());
            }
            catch (BdlDbException ex)
            {
                if (ex.ExceptionSubType == DbExceptionSubTypes.UniqueViolation)
                {
                    HandleUniqueViolation(ex);
                }
                else
                {
                    string errorMessage = "";
                    errorMessage = ex.Message;
                    Utils.ShowMessageBox(t_MessageBox.Attention, errorMessage);
                }
            }

            Controller.Busy = false;
    }
    catch (FieldException ex)
    {
        if (ex.FieldName == "FirstName")
        {
            TbFirstName.ValidationError = true;
            TbFirstName.ApplyErrorToolTip(ex.Message);
        }
    }
}

Here you're running the actual long running business operation that you have in a thread pool thread, but doing all of your error handling in the UI thread.

You can do the same thing throughout your application. Rather than putting everything into a background thread and then explicitly marshaling, just only ever execute operations in a background thread that should be entirely executed in a background thread.