Inner Exceptions, should we report them? Ever?

[Mike_R]

Ok, I'm a little confused about something...

I get the idea of Exceptions and the 'Inner Exception' or "innerEx", if you will. But the more I think about it, the more that I have trouble seeing any use for the 'innerEx' parameter for an exception class's constructor.

For example, I have my own custom Exception classes, and I have dutifully provided an 'innerEx' parameter for all my constructors. But I think I have never, ever used them when throwing an exception. The reason is that either I know what went wrong, in which case I throw the correct error type without any 'innerEx' provided, or I don't know, in which case I just re-throw via a 'Throw' call without any parameters in order to preserve the error stack trace when eventually handled by the higher-level error handler.

As an example, the System.ArgumentOutOfRangeException has a constructor overload that provides for a 'message' and an 'innerException'. Now, if the argument passed in is out of range, how can there be an inner exception at all? If the method is pre-checking the values and finds the argument out of range, then there can be no inner exception...

... I suppose that one could do post-validation, that is, don't pre-check, but surround the code with a Try-Catch-Finally block, and then only check for invalid arguments or the like within the Catch block. This is fine, and in this case there could theoretically be an inner exception to report. But would it make any sense to actually report that 'innerEx' here? For example, if the caller passed in, say, a null reference, or perhaps a negative integer where only positive numbers are valid, or some other invalid argument, then shouldn't the error itself be reported only? What purpose could it serve to report the error and in addition report the chaos this invalid argument caused when executed??

I was wondering what you all thought about this because I think I'm leaning towards removing the 'innerEx' parameters from the constructors to my exception classes. Certainly for the Argument-related exceptions.

Any thoughts?

 

[snarfblam]

Occasionally, I have found the InnerException property of a thrown exception very handy in the past when debugging. It might not serve much purpose in a distributed release build of an application, but it can help explain why an exception is being thrown during the development and debugging of an application.

I personally do not validate a lot of input for the sake of avoiding redundancy. I have a strong tenancy to not check array bounds because the CLR already does this for me, but I often catch the array bounds exception and throw my own Exception that better describes the situation. In this specific case it might not help a whole lot, but any time I catch an exception and throw another I specify the inner exception in case it helps explain what is going wrong. Suppose there was a hypothetical SerializationFailed exception for a class that serialized data to a file. Checking the inner exception might reveal that a file does not exist, or that it is read-only, or that the user does not have permission to a folder.

Classes like Exception really make me wish that there were a way to inherit constructors because it certainly is a pain, and hardly worth the effort, to write and re-write all of the constructor overloads there are for some classes. Since we don't have the luxury, though, we'll have to stick with alot of copy and paste code for our exceptions.

 

[Mike_R]

Ok, you make some very good points.

it can help explain why an exception is being thrown during the development and debugging of an application.

Yeah, this does make some sense. I'll have to think about this some more. Still, I feel like there are scenarios where reporting the inner exception just cannot be helpful...

I personally do not validate a lot of input for the sake of avoiding redundancy. I have a strong tenancy to not check array bounds because the CLR already does this for me, but I often catch the array bounds exception and throw my own Exception that better describes the situation. In this specific case it might not help a whole lot, but any time I catch an exception and throw another I specify the inner exception in case it helps explain what is going wrong.

Ok, yes, I often do the same. I call this "post-checking validaation" as opposed to "pre-checking validation". (I have not a clue if there is a standard name for this, or even if this is a standard thing to do...) But most of my code is naked, to be honest. Some does pre-validation, validating the argument values before executing any code. Some does post validation, wheby I run the code within a Try-Catch block and then only in the Catch section do I check for a null input or argument out of range. 99.9%+ the entries are correct, and so I prefer to only check for trouble when, well, I know there was trouble.

But in this case, my error handling tends to look something like this:

        private void myMethod(string[] stringsArray)
        {
            try
            {
                // Your code goes here...
            }
            catch
            {
                if (stringsArray == null)
                {
                    throw new ArgumentNullException("The array passed in is null.");
                }
                else if (stringsArray.Length  == 0)
                {
                    throw new ArgumentOutOfRangeException("The array passed in is zero-length.");
                }
                else
                {
                    // No clue, so just re-throw...
                    throw;
                }
            }
        }

In the above, I report what I can, but if "something else" happened, then I just pass on the exception as if it were never handled via the 'throw' call.

Suppose there was a hypothetical SerializationFailed exception for a class that serialized data to a file. Checking the inner exception might reveal that a file does not exist, or that it is read-only, or that the user does not have permission to a folder.

Yes, it could be file does not exist, string was not valid at all (bad format), CAS violation, who knows... So which argument do you call into which you pass the inner exception? I guess InvalidOperationException() sounds pretty good here. And I guess an inner exception could help clarify here.

Ok, I think you sold me.

Although, I'm still not sold on argument inputs. If I cleanly know that the "Argument is out of Range" or the "Argument is Null", what other inner exception information could possibly be relevant? Note though, that the System.ArgumentNullException and the System.ArgumentOutOfRangeException both have constructors that permit passing in an inner exception. But I'm having a hard time seeing the relevancy here.

Classes like Exception really make me wish that there were a way to inherit constructors because it certainly is a pain, and hardly worth the effort, to write and re-write all of the constructor overloads there are for some classes. Since we don't have the luxury, though, we'll have to stick with alot of copy and paste code for our exceptions.

Hmmm, I'm not sure that we can get around this. Another aproach would be protected virtual methods that let you override the 'message', and/or other values. Actually, the 'Message' is an overridable property.

This is a very good question though. I wonder why classes do not inherit the constructors? I think the issue must be that needing to hide a base class' constructor would be more frequent than wanting to pass it through exposed. For example, if an 'Employee' class inherited from the 'Person' class, the Employee class' constructor would need all the information that was required to make a new Person (FirstName, LastName, etc.) but would also require other info, like JobTitle, etc. So exposing the base class' constructor, which only includes FirstName and LastName would be a serious problem: it would allow the caller to create an incomplete/invalid class. So I think, more often than not, having the constructors be protected scope makes sense. It very well might not make as much sense for Exception classes, however, which more often than not have similar or identical construtors. But I don't think they can change the inheritance rules just for one set of classes, unfortunately...

 

[mskeel]

I think the inner exception exists so you don't lose your call stack. In the code above you are truncating the call stack when you throw a brand new exception. There is another exception constructor that takes a second argument of type exception. This way you can send you own specific exception while bundling up the real problem to go along with it. This allows you to pass up the call stack with you exception.

What you should be doing: (scroll right to see changes)

private void myMethod(string[] stringsArray)
{
   try
   {
      // Your code goes here...
   }
   catch (Exception ex)  // <-----
   {
      if (stringsArray == null)
      {
         throw new ArgumentNullException("The array passed in is null.", ex); // <-----
      }
     else if (stringsArray.Length  == 0)
     {
         throw new ArgumentOutOfRangeException("The array passed in is zero-length.", ex); // <-----
      }
      else
     {
         // No clue, so just re-throw...
        throw;
      }
    }
}

 

[Mike_R]

Yes! That's exactly right... However, I'm questioning this approach, at least as far as argument execptions are concerned, such as ArgumentException, ArgumentNullException, ArgumentOutOfRangeExeption, etc.

Here's the issue: of what value can the inner exception be if we know that the input(s) is/are invalid? The caller blew it, period.

For example, let's take a look at two different versions of very simmilar code. The first one uses pre-validation checking:

private void myMethod(string[] stringsArray)
{
    // Prevalidation Checking:

    if (stringsArray == null)
    {
        throw new ArgumentNullException("The array passed in is null.");
    }
    else if (stringsArray.Length  == 0)
    {
        throw new ArgumentOutOfRangeException("The array passed in is zero-length.");
    }

    // Arguments are valid, so run:
    try
    {
         // Your code goes here...
    }
    catch
    {
         // We don't need a Catch section at all:
         // But, just to be explicit:

        throw;
    }
}

So in the above, if we have an invalid argument, we throw an exception, but there is no inner exception to pass back to the caller. And to be honest, we don't need one: we simply tell the caller what went wrong, which is that the argument was no good.

Ok, now let's look at post-validation:

private void myMethod(string[] stringsArray)
{
    try
    {
        // Your code goes here...
    }
    catch
    {
        if (stringsArray == null)
        {
            throw new ArgumentNullException("The array passed in is null.");
        }
        else if (stringsArray.Length  == 0)
       {
            throw new ArgumentOutOfRangeException("The array passed in is zero-length.");
        }
        else
        {
            // No clue, so just re-throw...
            throw;
        }
    }
}

The above has the same exact functionality, at least as far as the caller is concerned. The caller will receive valid results, argument execptions, or some other exception for exactly the same reasons in both versions.

However, in the 2nd version, we have the opportunity to provide an inner exception. And your code showed exactly how to do it.

But the question is: should we?

I'll copy what I wrote in the first post:

But would it make any sense to actually report that 'innerEx' here? For example, if the caller passed in, say, a null reference, or perhaps a negative integer where only positive numbers are valid, or some other invalid argument, then shouldn't the error itself be reported only? What purpose could it serve to report the error and in addition report the chaos this invalid argument caused when executed??

So I think that for some exceptions, say an InvalidOperation execption or the like, passing along the inner exception could be valuable. But I can't think of a scenario where an inner exception reported along with an invalid argument exception makes any sense. But I guess I'm not 1,000% sold on my own argument, lol, so I'm posting here to see what others think...

 

[mskeel]

The problem I see is that it is not known why an exception was thrown when it is caught during "post-processing", all that is known is that an exception was thrown and that stringArray was null (for example). If it really is a condition that must be met for the proper execution of a method, then check it and bail as soon as possible. This way you avoid the case where stringArray was null but you threw an exception because you couldn't allocate new memory (or something similarly rare but possible). Fail early, fail often. In this way, exceptions will only be used to deal with exceptional problems.

Plus, in my opinion, you might as well sprinkle some goto's in that big pile of spaghetti code. Where's the crucial error checking? It's at the bottom, but you won't know that unless there's a problem!

 

[Nerseus]

I generally try to enforce arguments with Trace.Assert, if I want to catch it at debug time. To check for a null string, I'd use the following right at the top (before I use "Try"):
Trace.Assert(stringsArray == null, "stringsArray == null");

Some prefer Debug.Assert, but I'd rather get this error in QA than nothing.

As for inner exceptions - I've never used them myself. I've looked at them while debugging and some unhandled exception is thrown. Within my catch blocks I generally log an error to our standard logging and then use "throw;" to re-throw the error.

In other places, I might throw a new exception (such as new ArgumentException) rather than Trace.Assert - but that wouldn't be from within a Catch block. I prefer the new style of Trace.Assert, just because it's easier to remember for me.

In general, I don't use try/catch that much - only around code that might possibly have an unexpected error such as every call to the DB or to a webservice. If I'm processing data in a DataSet or doing something with UI code, I wouldn't expect an exception and wouldn't want a try/catch just because I'm unsure. That's our general guideline where I work - don't use try/catch unless there's a call that may throw an exception that you couldn't have prevented with an "if" test.

-ner

 

[Mike_R]

The problem I see is that it is not known why an exception was thrown when it is caught during "post-processing", all that is known is that an exception was thrown and that stringArray was null (for example). If it really is a condition that must be met for the proper execution of a method, then check it and bail as soon as possible. This way you avoid the case where stringArray was null but you threw an exception because you couldn't allocate new memory (or something similarly rare but possible). Fail early, fail often. In this way, exceptions will only be used to deal with exceptional problems.

Agreed... argument validation should happen up front. But in this case there is no inner exception to include in the error message. More and more I think I'm comfortable leaving the inner exceptions out of my argument exception classes.

Plus, in my opinion, you might as well sprinkle some goto's in that big pile of spaghetti code. Where's the crucial error checking? It's at the bottom, but you won't know that unless there's a problem!

Ouch... Ok, I do agree that Argument validation should be done up front, no problem there. I was stretching the point to show that even if you did have an inner exception to provide with your argument execption, well, even then I can't see the point in providing it.

Post-checking within a Catch execption is very normal, of course, it's just not normal for argument validation. Still, I think one could make the case for it, but that would side-track the issue here...

I generally try to enforce arguments with Trace.Assert, if I want to catch it at debug time. To check for a null string, I'd use the following right at the top (before I use "Try"):
Trace.Assert(stringsArray == null, "stringsArray == null");

Ugh, the inverse-logic ("report if false") mentality of Trace.Assert() drives me up the wall... For example, shouldn't your example be Trace.Assert(stringsArray != null, "stringsArray == null"); Maybe I'm a dolt, but I find this surpisingly tricky for me to get my head around.

But syntax asside, that does sound pretty good. I think I achieve more or less the same by trapping the unhandled AppDomain.UnhandledException and Application.ThreadException. From there you can report a message box to the user, or just log it. But if you pre-validate and find a whoopsie, then Trace.Assert() on the spot does look pretty good. Well, if it's fatal, I guess. Yeah, that is a problem... What if the caller has error handling and will quietly handle a failure and try again, or do something else... One has to be certain that the failure is pretty fatal (ok, sure, most exceptions are) before you side-step the error handling and annoy the user, yeah?

As for inner exceptions - I've never used them myself. I've looked at them while debugging and some unhandled exception is thrown. Within my catch blocks I generally log an error to our standard logging and then use "throw;" to re-throw the error.

Yes, this is what I do. So much so that I've started to question inner exceptions at all... But I think I could see where say a file is locked (CAS access, or in used by another user, or...) and so one *could* throw, say, an InvalidOperationException, providing an inner exception along with it. But for invalid argument exeptions, I really have a hard time seeing the use for inner exceptions I think.

In other places, I might throw a new exception (such as new ArgumentException) rather than Trace.Assert - but that wouldn't be from within a Catch block. I prefer the new style of Trace.Assert, just because it's easier to remember for me.

What's the "new style"? (Sorry I don't use this command myself.)

In general, I don't use try/catch that much - only around code that might possibly have an unexpected error such as every call to the DB or to a webservice.

For me as well. I'm almost embarrassed to say how naked most of my code is. On the other hand, if it fails, it's generally because of bad inputs.

If I'm processing data in a DataSet or doing something with UI code, I wouldn't expect an exception and wouldn't want a try/catch just because I'm unsure. That's our general guideline where I work - don't use try/catch unless there's a call that may throw an exception that you couldn't have prevented with an "if" test.

Yeah, this sounds smart.


Thanks for the tips guys...

 

[PlausiblyDamp]

I must admit that in the past I tended to be a big fan of the various Assert mechanisms in most languages, these days I virtually never use them as I tend to get quite fanatical about unit tests as a way to catch these problems.

Getting back to the topic though... I honestly can't remember a single time I have ever either checked the InnerException property or thrown an Exception where I felt the need to provide one. I can't think of many situations where effectively changing the exception to a different one from the actual real problem is ever going to make thinks clearer.

In a similar vein I often don't check for things like nulls so I can throw an exception if the result of me not checking is simply the same exception gets thrown anyway.

In practice i tend to find the number of try ... catch blocks is far outweighed by try ... finally (or using statements really) as in most places when an exception is thrown there is either very little that can be done or it's serious enough that an application level handler is more suitable as there is nothing much you can do but indicate the connection to database / server / whatever has been lost.

 

[snarfblam]

I honestly can't remember a single time I have ever either checked the InnerException property or thrown an Exception where I felt the need to provide one.

If you can't figure out why an exception is being thrown, InnerException is one of many clues you can look at. You can get along without it but it can be real handy once in a blue moon (see below).

I can't think of many situations where effectively changing the exception to a different one from the actual real problem is ever going to make thinks clearer.

Look at TargetInvocationException: when you use reflection to invoke a method and an exception is thrown by reflected code, you don't get the original exception, you get a TargetInvocationException where the original exception is specified as the InnerException. Why is this useful? There is a good chance that you want to separate exceptions thrown by reflected code from exceptions raised in your own code. In an earlier post I mentioned a hypothetical SerializationException. Like the TargetInvocationException, this would wrap an exception for the sake of specifiying or clarifying the context of the exception.

In this light, though, it would be more reasonable to implement the InnerException property further down the road, rather than in the base class.

In a similar vein I often don't check for things like nulls so I can throw an exception if the result of me not checking is simply the same exception gets thrown anyway.

I couldn't agree with you more, but it is important to make sure that if you code like this then there aren't any harmful side effects that could be avoided by validating arguments before trying to use them. Ideally, when an exception is thrown it is best for there to be no result from the executed code to maximize the possibility of a recovery.

 

[Nerseus]

@Mike: By "new style" I just meant using Trace.Assert vs a standard "if (some condition) throw new Exception()".

I wish I had more time to spend on getting some initial unit testing framework in place. If so, I think I'd end up preferring unit tests over Trace.Assert as I should be able to validate my data much easier that way and it would keep all those validation/assumptions in one place rather than within a method.

-ner

 

[Mike_R]

@Mike: By "new style" I just meant using Trace.Assert vs a standard "if (some condition) throw new Exception()". I wish I had more time to spend on getting some initial unit testing framework in place. If so, I think I'd end up preferring unit tests over Trace.Assert as I should be able to validate my data much easier that way and it would keep all those validation/assumptions in one place rather than within a method.

Ah, ok, now I see what you mean: you Assert if the input or output is outside the allowable range from a 'unit test' point of view. That makes perfect sense. Well, a separate set of unit tests would certainly be more robust (if one had the time!), but there's something nice about this "unit test" being built right into the code itself, if you will. Thanks Ner.


Btw, I started scratching my head on this "Post-validation" idea of mine, wondering if it really did make any sense. I think that mostly it doesn't, but I looked through my code and found that I was using it where validating the input was an expensive check. In particular, I have some calls to COM objects that can fail (among other reasons) if the COM object's RCW is no longer valid (maybe some idiot called Marshal.ReleaseComObject() or the like) or if the RCW is valid, but is a invalid object (say the user closed the workbook to which you held a reference). Either of these cases is sure to cause a failure for any operation on that object. However, testing for an invalid COM object is expensive. And your code should never get into this situation in the first place, but if you have an exeption, then, well, this is certainly the first thing that I would check for.

Since testing if the COM object is expensive, I test for this only in the Catch section. That is, I do "post-validation" checking not "pre-validation". Then if the object is invalid, the code throws an invalid argument exception. (And no, I don't pass along an inner exception; since the object itself was invalid, that's all the caller needs to know). On the other hand, if the COM object is valid, then, well, "something else" happend and I just call 'throw' so that the stack-trace remains in tact.

Anyway, if any of this makes sense to anyone... :-\