Is this a bad practice to catch a non-specific exception such as System.Exception? Why?

Question

I am currently doing a code review and the following code made me jump. I see multiple issues with this code. Do you agree with me? If so, how do I explain to my colleague that this is wrong (stubborn type...)?

• Catch a generic exception (Exception ex)
• The use of "if (ex is something)" instead of having another catch block
• We eat SoapException, HttpException and WebException. But if the Web Service failed, there not much to do.

Code:

try
{
    // Call to a WebService
}
catch (Exception ex)
{
    if (ex is SoapException || ex is HttpException || ex is WebException)
    {
        // Log Error and eat it.
    }
    else
    {
        throw;
    }
}

Answer 1

The mantra is:

• You should only catch exceptions if you can properly handle them

Thus:

• You should not catch general exceptions.

---

In your case, yes, you should just catch those exceptions and do something helpful (probably not just eat them--you could throw after you log them).

Your coder is using throw (not throw ex) which is good.

This is how you can catch multiple, specific exceptions:

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    // Log Error and eat it
}
catch (HttpException ex)
{
    // Log Error and eat it
}
catch (WebException ex)
{
    // Log Error and eat it
}

This is pretty much equivalent to what your code does. Your dev probably did it that way to avoid duplicating the "log error and eat it" blocks.

Answer 2

You should usually still catch generic exceptions in a global handler (which is the perfect place to log unexpected Exceptions), but otherwise as said before, you should only catch specific exception types in other places if you plan to do something with them. The catch blocks should look for those exception types explicitly, not as the code you posted does.

Answer 3

The problem with catching and re-throwing the same exception is that, although .NET does its best to keep the stack trace intact, it always ends up getting modified which can make tracking down where the exception actually came from more difficult (e.g. the exception line number will likely appear to be the line of the re-throw statement rather than the line where the exception was originally raised). There's a whole load more information about the difference between catch/rethrow, filtering, and not catching here.

When there is duplicate logic like this, what you really need is an exception filter so you only catch the exception types you're interested in. VB.NET has this functionality, but unfortunately C# doesn't. A hypothetical syntax might look like:

try
{
    // Call to a WebService
}
catch (Exception ex) if (ex is SoapException || ex is HttpException || /* etc. */)
{
    // Log Error and eat it
}

As you can't do this though, what I tend to do instead is use a lambda expression for the common code (you could use a delegate in C# 2.0), e.g.

Action<Exception> logAndEat = ex => 
    {  
        // Log Error and eat it
    };

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    logAndEat(ex);
}
catch (HttpException ex)
{
    logAndEat(ex);
}
catch (WebException ex)
{
    logAndEat(ex);
}

Answer 4

I am currently doing a code review and the following code made me jump. I see multiple issues with this code. Do you agree with me?

Not totally, see below.

• Catch a generic exception (Exception ex)

In general, catching a generic exception is actually ok as long as you rethrow it (with throw;) when you come to the conclusion that you can't handle it. The code does that, so no immediate problem here.

• The use of "if (ex is something)" instead of having another catch block

The net effect of the catch block is that only SoapException, HttpException, etc. are actually handled and all other exceptions are propagated up the call stack. I guess functionality-wise this is what the code is supposed to do, so there's no problem here either.

However, from a aesthetics & readability POV I would prefer multiple catch blocks to the "if (ex is SoapException || ..)". Once you refactor the common handling code into a method, the multiple catch blocks are only slightly more typing and are easier to read for most developers. Also, the final throw is easily overlooked.

• We eat SoapException, HttpException and WebException. But if the Web Service failed, there not much to do.

Here possibly lurks the biggest problem of the code, but it's hard to give advice without knowing more about the nature of the application. If the web service call is doing something that you depend on later then it's probably wrong to just log & eat the exceptions. Typically, you let the exception propagate to the caller (usually after wrapping it into e.g. a XyzWebServiceDownException), maybe even after retrying the webservice call a few times (to be more robust when there are spurious network issues).

Answer 5

I don't think this is such a bad thing in this case, but I also do something similar in my code with exceptions that can be safely ignored being caught and the rest being re-thrown. As noted by Michael's response, having each catch being a separate block could cause some readability issues which are prevented by going this route.

In regards to pointing this out to your colleague, I think you would have a hard time convincing them that this is the wrong way of doing things - even more so if they are stubborn - because of the potential readability issues with doing things the other way. Since this version is still throwing the generic exception's that can't be handled it is in keeping with the spirit of the practice. However, if the code was in line with the following:

try
{
  // Do some work
}
catch (Exception ex)
{
  if (ex is SoapException)
  {
    // SoapException specific recovery actions
  }
  else if (ex is HttpException)
  {
    // HttpException specific recovery actions
  }
  else if (ex is WebException)
  {
    // WebException specific recoery actions
  }
  else
  {
    throw;
  }
}

Then I think you would have a bit more of a reason to be concerned as there is no point in doing work for a specific exception by checking for it in a general exception block.

Answer 6

Sometimes that is to only way to handle "catch every exception" scenarios, without actually catching every exception.

I think sometimes, say, lowlevel framework / runtime code needs to make sure that no exception is ever escaping. Unfortunately, there is also no way the framework code can know which exceptions are raised by the code executed by the thread.

In that case a possible catch block could look like this:

try
{
   // User code called here
}
catch (Exception ex)
{
   if (ExceptionIsFatal(ex))
     throw;

   Log(ex);
}

There are three important points here, however:

1. This isn't something for every situation. In code reviews we are very picky about places where this is actually neccessary and thus allowed.
2. The ExceptionIsFatal() method assures that we don't eat exceptions which should never be swallowed (ExecutionEngineException, OutOfMemoryException, ThreadAbortException, etc.)
3. What is swallowed is carefully logged (event-log, log4net, YMMV)

Typically, I'm all for the practice of letting uncaught exceptions simply "crash" the application by terminating the CLR. However, especially in server applications, this is sometimes not sensible. If one thread encounters a problem which is deemed non-fatal, there is no reason in ripping the whole process down, killing off all other running requests (WCF, for example, handles some cases this way).