1

I know that often catching all exceptions (C#: catch(Exception exception){...}) is deemed bad practice.

However, I believe that there are situations where it is perfectly reasonable to do it. For example, if I have a number of operations to perform:

foreach(var operation in operationBatch)
    operation.Do();

and a batch should either succeed as a whole, or fail as a whole, does this seem reasonable?

var rollBackStack = new Stack<Operation>(operationBatch.Length);
try{
  foreach(var operation in operationBatch)
  {
      operation.Do();
      rollBackStack.Push(operation); // assuming operation is atomic and cannot both apply changes AND fail
  }
} catch(Exception exception) // whatever went wrong...
{
    foreach(var ranOperation in rollBackStack)
      ranOperation.Undo();

    throw; // or throw new MyDomainException("relevant message", exception);
}

I guess a generic version of the question would be: is catching all exceptions only to perform some action and rethrowing an acceptable practice, or is there another way that's recommended? I can think of something like

var completed = 0;
try{
  foreach(var operation in operationBatch)
  {
      operation.Do();
      ++completed;
  }
} finally {
    if(completed != operationBatch.Length)
        foreach(var ranOperation in operationBatch.Take(completed).Reverse())
          ranOperation.Undo();

    // no need to rethrow from finally
}

But this seems like simply unnecessarily more complex version of the first snippet.

  • 1
    Your first version is pefectly OK. You know you can't complete the whole operationsList, so you roll back. That's a perfectly legal reason for catching an exception, doing something and re-throwing because your caller needs to know that the batch didn't execute. – Ralf Kleberhoff Sep 11 '17 at 10:22
  • I've said it before: Don't get hung on CA1031 - it's a very incomplete rule. – Martin Ba Sep 20 '17 at 8:42
3

You're OK.

From the documentation for CA1031:

CA1031: Do not catch general exception types

Cause

A general exception such as System.Exception or System.SystemException is caught in a catch statement, or a general catch clause such as catch() is used.

How to Fix Violations

To fix a violation of this rule, catch a more specific exception, or rethrow the general exception as the last statement in the catch block.

It seems the guidance on this particular rule is that it is OK to catch a general exception as long as you rethrow it. Your design is consistent with this guidance.

Note: There is a signficant difference between these two statements:

throw; 

throw new MyDomainException("relevant message", exception);

In most cases you should use the former, which preserves the stack trace and original exception type so that "outer" catch blocks can distinguish between a system error like out of memory (which they usually will not want to catch, since they can't really handle them) and domain exceptions (which they probably can).

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.