Mad, Beautiful Ideas
C# Style Question: Exception Handling in Callbacks

Recently, a coworker expressed frustration over the way he was having to handle errors in WCF Asynchronous callbacks. For Synchronous WCF calls, fault handling is simple:

        var webService = new WebServiceWrapper();
        
        try {
            var answer = webService.callMethodSynchronous(arguments);
        
            // Handle answer 
        } catch (FaultException) {
            // Handle Specific Exception
        } catch (Exception) {
            // Handle General Exception
        } finally {
            // Clean-up
        }
        

Prety basic and normal exception handling code, textbook really.

However, in WCF, when using an async callback the suggested code looks more like this:

        var webService = new WebServiceWrapper();
        
        webService.methodAsyncCompleted += (sender, response) => {
            if (response.Error == null) {
                // Handle Success Case
            } else {
                Type exceptionType = response.Error.GetType();
                if (exceptionType == typeof(FaultException)) {
                    // Handle Specific Exceptions
                } else { 
                    // Handle General Exception
                }
            }
            // General Clean-up
        };
        webService.callMethodASync(arguments);
        

The if-statements can be more concisely written as follows:

        var webService = new WebServiceWrapper();
        
        webService.methodAsyncCompleted += (sender, response) => {
            if (response.Error == null) {
                // Handle Success Case
            } else {
                if (response.Error is FaultException) {
                    // Handle Specific Exceptions
                } else { 
                    // Handle General Exception
                }
            }
            // General Clean-up
        };
        webService.callMethodASync(arguments);
        

I jokingly suggested rethrowing the Exception:

        var webService = new WebServiceWrapper();
        
        webService.methodAsyncCompleted += (sender, response) => {
            if (response.Error == null) {
                // Handle Success Case
            } else {
                try {
                    throw response.Error;
                } catch (FaultException) {
                    // Handle Specific Exceptions
                } catch (Exception) {
                    // Handle General Exception
                }
            }
            // General Clean-up
        };
        webService.callMethodASync(arguments);
        

When we wrote this up really quick, our reaction was similar. It really feels like something we shouldn't do, but it also seems a bit cleaner. On one hand, a Catch block is a great way to determine the exact type of any class deriving from System.Exception (.NET doesn't allow you to throw Non-Exceptions, other platforms may not have that limitation). But is it worth the potential style faux-pas? And are there any other downsides to this. To see, I checked the IL of a simple application.

For the if-else typeof case:

        IL_0000:  ldarg.0 
        IL_0001:  callvirt instance class [mscorlib]System.Type class [mscorlib]System.Exception::GetType()
        IL_0006:  stloc.0 
        
        IL_002b:  ldloc.0 
        IL_002c:  ldtoken [mscorlib]System.ArgumentException
        IL_0031:  call class [mscorlib]System.Type class [mscorlib]System.Type::GetTypeFromHandle(valuetype [mscorlib]System.RuntimeTypeHandle)
        IL_0036:  bne.un IL_004f
        

For those not fluent in IL, I'll give a line-by-line:

  1. Load the first argument onto the evaluation stack, prepare to use it.
  2. Call System.Exception::GetType(), which returns an instance of System.Type
  3. Store the return value of the previous operation into location 0
  4. Load the value in location 0 onto the evalution stack
  5. Load the token representing System.ArgumentException onto the evalution stack
  6. Call GetTypeFromHandle to get a Type reference to the token loaded previously, this will use the top of the evaluation stack, and will put the return valeu on it.
  7. If the two values are not equal (bne) than jump to IL_004f, which happens to be the instruction where our Else block begins

For the if-else is case:

        IL_0000:  ldarg.0 
        IL_0001:  isinst [mscorlib]System.ApplicationException
        IL_0006:  brfalse IL_001f
        
  1. Load argument 0 onto execution stack
  2. Take the top of the execution stack, check if it's an instance of System.ApplicationException, put a boolean on the execution stack
  3. If false is on top of the execution stack, go to IL_001f

And now for the try-catch version:

        .try { // 0
         IL_0000:  ldarg.0 
         IL_0001:  throw 
         IL_0002:  leave IL_0046
        
        } // end .try 0
        catch class [mscorlib]System.ApplicationException { // 0
         IL_0007:  pop 
         // Do whatever
         IL_0017:  leave IL_0046
        
        } // end handler 0
        

Breakdown:

  1. Load the value of the Exception on the evaluation stack
  2. Throw it
  3. This will never be hit in this example. It would jump to the finally block.
  4. Clear the value off the top of the stack
  5. Go to the Finally block.

It certainly is more IL, but from a pure IL perspective, it doesn't seem...bad, except for the fact that it does create a LOT more blocks, and leaving those blocks is a more expensive operation, since the Try and Catch blocks are considered 'protected' regions of code. Setting up and entering these might be more expensive, I'm not really sure, and I'd love someone more familiar with .NET Runtime internals to post their take on it.

After looking over both the IL and the C# code, I'm left thinking that using the 'is' operator is the best option in this case. It generates the least amount of code. The simplest code. The only downside is that it has you testing exceptions using code that doesn't resemble exception handling anywhere else. For that reason alone, the Try-Catch mechanism may really be best, though I am leaning toward simply using 'is'.

What are your thoughts?