Async and error-handling

Question

I need to delete a file when an error occurs during loading it. It is not allowed to perform async operations in a catch block.

This has lead me to writing the following code:

 public async Task LoadFromStorageFile(string fileName)
 {
 StorageFile file = null; 
 Exception ex1 = null;
 try
 {
 var folder = await ApplicationHelper.GetApplicationSaveFolder();
 file = await folder.GetFileAsync(fileName);
 // ...
 }
 catch (Exception ex)
 {
 ex1 = ex;
 }
 try
 {
 if (ex1 != null && file != null)
 {
 await file.DeleteAsync(StorageDeleteOption.PermanentDelete);
 }
 }
 catch (Exception)
 {
 //...
 }
 finally
 {
 if(ex1 != null)
 throw ex1;
 }
 }

Is there a nicer way of doing this? The rethrown exception especially bugs me.

• svick: "It is not allowed to perform async operations in a catch block." Not yet, it's probably going to be allowed in C# 6.0.

Answer 1

I need to delete a file when an error occurs during loading it.

In other words:

Happy path:

• Load a file
• Move on

Unhappy path:

• Load a file, exception is thrown
• Delete the file
• Move on

Exceptional path:

• Load a file, exception is thrown
• Delete the file, exception is thrown
• Handle exception, move on

---

From these points, it appears we'd need a task that loads a file. If that task fails, we need to try to handle the unhappy path gracefully.

I haven't written async code with async/await, but I'd probably get away with this using a Task that loads the file, and then decide whether I'd .ContinueWith another Task that deletes the file, and then decide how to handle the exceptional path where that other task would have failed.

---

Signature

public async Task LoadFromStorageFile(string fileName)

This StackOverflow answer refers to an authoritative source for async method naming conventions:

The Task-based Asynchronous Pattern (TAP) dictates that methods should always return a Task<T> (or Task) and be named with an Async suffix; this is separate from the use of async. Certainly, Task<bool> Connect() (or async Task<bool> Connect()) will compile and run just fine, but you won't be following the TAP naming convention.

The signature for Task LoadFromStorageFile(string) would follow the naming convention for the pattern, by appending "Async" to the method's name:

public async Task LoadFromStorageFileAsync(string fileName)

---

Async/Await Exception Handling

As I've found in this StackOverflow answer, an exception thrown in an async method will bubble up to the caller, so whoever called await LoadFromStorageFileAsync("somefile.txt"); can know whether to move on, or to handle the unhappy path.

It is not allowed to perform async operations in a catch block.

Indeed. There's a question on Stack Overflow, with a nice answer from one of our top reviewers here on CR, that shows how that limitation can be circumvented. In a nutshell:

bool loadSucceeded;
try
{
 var folder = await ApplicationHelper.GetApplicationSaveFolder();
 file = await folder.GetFileAsync(fileName);
 // ...
 loadSucceeded = true;
}
catch
{
 loadSucceeded = false;
}

---

LoadFromStorageFileAsync can then look like this:

public async Task LoadFromStorageFileAsync(string fileName)
{
 bool loadSucceeded;
 StorageFile file;
 try
 {
 // happy path
 var folder = await ApplicationHelper.GetApplicationSaveFolder();
 file = await folder.GetFileAsync(fileName);
 // ...
 loadSucceeded = true;
 }
 catch(Exception exception)
 {
 // log exception
 loadSucceeded = false;
 }
 if (!loadSucceeded && file != null) 
 {
 try
 {
 // unhappy path
 await file.DeleteAsync(StorageDeleteOption.PermanentDelete);
 deleteSucceeded = true;
 }
 catch(Exception exception)
 {
 // exceptional path
 // log exception
 }
 }
}

This code doesn't rethrow whatever exception caused loadSucceeded to be false - rethrowing that exception in the unhappy path makes no sense, as was said before, that finally block is not needed. Instead, you could catch(Exception exception) (or anything more specific) in the happy path's catch block, and write a trace, a debug, or a warning log entry with the exception and its stack trace. At that point, it's taken care of, there's no need to re-throw it.

Answer 2

Couple random observations:

This initialization is redundant:

 StorageFile file = null; 
 Exception ex1 = null;

Because all reference type default to null, this is rigorously equivalent:

 StorageFile file; 
 Exception ex1;

Your method is doing 2 things, it should be broken down into 2 methods.

throw ex1 is kissing your stack trace goodbye, and throwing an exception in a finally block is rather... unusual. Why keep a reference to an exception that you're going to be throwing anyway? If the first try block throws, that's it, I'd be expecting file to be null and to not be entering the condition in the second try block.

 catch (Exception)
 {
 //...
 }

This no-op catch block is swallowing any & all exceptions - don't do that. If you can't handle an exception in a meaningful and useful way, then let it blow up! Let the caller do something about an unhandled exception, don't just pretend everything went fine! The minimum you could do is something like this (assuming you're using a logging framework):

 catch (Exception exception)
 {
 _logger.WarnException(exception);
 }

• Tory: I'll contest Type value = null; is equivalent to Type value; since @thumbmunkeys is using ex1 in a finally block, it MUST be set to something before ever getting there. Try it.
• thumbmunkeys: thanks for your remarks. I know not to swallow exceptions, I left non relevant parts out of my code. You cannot access an uninitialized local variable in c#, so file and ex1 have to be initialized. folder is in this context indeed not neccessary (in my real code it isn't). I agree the finalizer block is superflous. Could you give a suggestion how to divide the code, so that the stacktrace is not lost?
• Mathieu Guindon: @Tory good point