Catching errors with .NET Task Parallel Library

Question

Here are the two alternatives i tried for catching errors, they both seem to do the same thing.. but is one preferable over the other and why ?

Alt 1:

private async void BtnClickEvent(object sender, RoutedEventArgs e)
{

    try
    {
        Task t = Task.Run(() =>
            {
                _someObj.SomeMethod();
            });
        await t; //wait here, without blocking...
    }
    catch (Exception ex)
    {
        string errMsg = ex.Message + Environment.NewLine;
        errMsg += "some unhandled error occurred in SomeMethod";
        Log(errMsg);

        return; //<-- bypass below code on error...
    }

    //other code below... does not execute...
    DoSomethingElse();

}

Alt 2:

private async void BtnClickEvent(object sender, RoutedEventArgs e)
{

    bool errOccurred = false;

    Task t = Task.Run(() =>
        {
            try
            {
                _someObj.SomeMethod();
            }
            catch (Exception ex)
            {
                string errMsg = ex.Message + Environment.NewLine;
                errMsg += "some unhandled error occurred in SomeMethod";
                Log(errMsg);

                errOccurred = true;

            }//end-Catch
        });
    await t; //wait here, without blocking...
    if (errOccurred) return; //<-- bypass below code on error...

    //other code below... does not execute...
    DoSomethingElse();  

}

Answer 1

As with anything it depends.

I'd say refactor the Task.Run() section into a separate async Task method, much like Sriram Sakthivel's answer, is in general a good thing. It avoids the use of a captured bool in the lambda as in version 2, and it lets you write code that expresses intent more concisely.

That said, I would carefully consider if the "catch all -> log -> ignore" pattern is what you want. In general: catch specific exceptions and handle them specifically. For all other exceptions, you might log them, but still rethrow them with "throw;" or "throw new MoreSpecificException(originalException);".

With that in mind I would suggest that if you do the catch all approach you should do the catch all as in version 1.

To keep readability high, make the code concise with clear intent, and be explicit about handling exceptions, I would write it something like this:

private async void BtnClick(object sender, RoutedEventArgs e)
{
    try
    {
        if (await TryDoSomethingAsync())
        {
            DoSomeMoreStuff();
        }
    }
    catch (Exception ex)
    {
        // I am sure it is fine that any and all exceptions can be logged and ignored.
        Log(ex);

        // And maybe even notify the user, since I mean, who monitors log files anyway?
        // If something that shouldn't go wrong goes wrong, it's nice to know about it.
        BlowUpInYourFace(ex);
    }
}


private async Task<bool> TryDoSomethingAsync()
{
    return await Task.Run<bool>(() =>
    {
        try
        {
            _myService.DoSomething();
        }
        catch (SomeKnownException ske)
        {
            // An expected exception which is fine to ignore and return unsuccessful.
            Log(ske);
            return false;
        }
        catch (SomeOtherKnownException soke)
        {
            // Expected exception that indicates something less trivial, but could be more precise.
            throw new MyMorePreciseException(soke);
        }

        // Nothing went wrong, so ok.
        return true;
    });
}

Answer 2

It's better to refactor the code than putting it all at the same place.It's better to catch the exception within your delegate if all you need to do is log it.

private async void BtnClickEvent(object sender, RoutedEventArgs e)
{
  await Task.Run(() =>
        {
            try
            {
               DoSomeWork();
            }
            catch (Exception ex)
            {
                log.Error(ex.Message);
            }
        });
}

However If you have another method DoSomethingElse() which might be affected by the outcome of the Task.It's better to wrap try catch around await

private async void BtnClickEvent(object sender, RoutedEventArgs e)
{
    try
    {
        await Task.Run(() =>
        {
            try
            {
                DoSomeWork();
            }
            catch (Exception ex)
            {
                log.Error(ex.Message);
            }
        });

        DoSomethingElse();

     }
     catch(Exception ex)
     {

     }
}

Answer 3

Better option is to refactor part of the code into a separate method returning a bool indicating that whether to proceed or not.

private async void BtnClickEvent(object sender, RoutedEventArgs e)
{
    bool success = await SomeMethodAsync();
    if (!success)
    {
        return;
    }

    //other code below... does not execute...
    DoSomethingElse();
}

private async Task<bool> SomeMethodAsync()
{
    try
    {
        await Task.Run(() => _someObj.SomeMethod());
        return true;
    }
    catch (Exception ex)
    {
        string errMsg = string.Format("{0} {1}some unhandled error occurred in SomeMethod",
        ex.Message, Environment.NewLine);
        Log(errMsg);          

        return false;
    }
}