ThrowsAdvice

[spmva]

As this is my first post since the release of 1.0, I'd like to give a quick thanks to all those who contributed to its development.

As for my question, I hope it's not too stupid [smile]. It is the end of the day in my part of the world and the old brain isn't firing like it was earlier.

I'm working with IThrowsAdvice in an attempt to break out some complex exception handling that's scattered throughout parts of my code. After configuring my implementation of IThrowsAdvice and running my app, I found that it just didn't work.

The good news is that I'm pretty sure that I know the reason why it doesn't work and have a solution, but my investigation into the problem took me into the source code for the ThrowsAdviceInterceptor class, specifically to the InvokeMethod and this code:
try
{
return mi.Proceed();
}
catch (Exception ex)
{
if (ex.InnerException != null)
{
ex = ex.InnerException;
}

MethodInfo handlerMethod = GetExceptionHandler(ex);
if (handlerMethod != null)
{
InvokeHandlerMethod(mi, ex, handlerMethod);
}
throw ex;
}
My problem seems to be related to the fact that when the Proceed method is called and the exception is thrown within that invoked method, the following code:
if (ex.InnerException != null)
{
ex = ex.InnerException;
}

Only goes one .InnerException deep, when the exception I was trying to capture was really at .ex.InnerException.InnerException.

I was just wondering if there was any reason why looking for a single InnerException was considered deep enough. Why not drill down until you hit the bottom most InnerException or why not test every single one? Perhaps there is some good design principle that I should be aware of here, maybe?

Best regards,

Steve

[Rick Evans]

Hiya Steve

Yes, that would appear to be counter-intuitive to say the least. In fact, its a bona-fide bug.... the exception chain must be walked, and the first (if any) matching handler that is found must be used. Gee, the bugs are all firing in today :oops: The fixes will also be firing in though. Thanks for spotting this... and do feel free to use the JIRA installation to report bugs like this in the future.

To wit, I have created a JIRA issue to track the resolution of this issue.

http://opensource2.atlassian.com/projects/spring/browse/SPRNET-178

Ciao
Rick

[Mark Pollack]

Hi,

Hmm this is probably interesting to feedback into spring.java as well since chained exceptions are not handled there in the way we are suggesting now.

- Mark

[Aleks Seovic]

I'm not sure they should be handled the way it was proposed.

From my standpoint, only the actual exception caught should be intercepted by default, without any inspection of the nested exceptions. That still means that we might have a bug as is, because I don't understand the reason for inner exception check and retrieval.

What we might want to do is add an optional parameter that would tell us whether to walk exception hierarchy, but I'm not 100% sure of that either -- it might introduce large overhead for the dubious benefit.

My question is, why not simply define advice methods for the actual exception types that can be possibly thrown by the advised code?

- Aleks

[Rick Evans]

Hiya

I quizzed the AOP masters (Aleks and Rob), and the official line is that chained exceptions are not eligible for interception by throws advice. That is the current, official line as of the 1st of September... it may well change, but not right now and certainly not for the 1.0 final release in two weeks time (well, probably)

More clearly, only the top level exception thrown from the invocation of the target joinpoint can ever be handled by throws advice.

This means that the issue you are having is not going to be addressed (fixed). It also means that the current Spring.NET implementation of the ThrowsAdviceInterceptor that checks for an InnerException is wrong too... it must only be operating with the exception as-is, and must not check for the presence of an InnerException at all. It must also preserve the stack trace at the end of the handling, but that's a side issue.

I'm not an AOP master, so I will defer to my betters in this instance (I have appended the fix I was going to commit below). Perhaps if you voted for the issue in JIRA it would get sorted... I could also raise an equivalent issue in the Spring (for Java) JIRA if you wish.

Ciao
Rick

public object Invoke(IMethodInvocation invocation)
{
try
{
return invocation.Proceed();
}
catch (Exception ex)
{
MethodInfo handlerMethod = GetExceptionHandler(ex);
while (handlerMethod == null && ex.InnerException != null)
{
ex = ex.InnerException;
handlerMethod = GetExceptionHandler(ex);
}
if (handlerMethod != null)
{
InvokeHandlerMethod(invocation, ex, handlerMethod);
}
throw;
}
}

[spmva]

I think that the conclusion that you all came to makes sense. I didn't think that walking the exception hierarchy was a necessarily good idea either, mererly an option. Of course, in listing all those possiblities, I left out the most obvious: just evaluate the exception for what it is.

As for the question from Aleks:
My question is, why not simply define advice methods for the actual exception types that can be possibly thrown by the advised code?I guess the issue arose when I was working with a IBatisNet.DataAccess.DaoManager returned DAO. For a particular method, my DAO had some simple logic in it to test for proper parameter formatting. If the test failed it would throw an exception, for example: ArgumentOutOfRangeException. I guess the DaoManager returns some kind of proxy that wraps my original implementation, because when that exception throws, the ArgumentOutOfRangeException is wrapped in an IBatisNet.DataAccess.Exceptions.DataAccessExceptio n, which is what the ThrowsAdvice picks up. Therefore, without examining the contents of the InnerException, I wouldn't know what really caused the exception, and for my purposes, I would want to process a client problem, like a bad parameter, differently than a bad database connection or query. This processing is the same across all my DAOs so it seemed like a good place for AOP.

I'm not an AOP expert, so I can only guess at what the best practice would be here, given your decision to only examine the top-most exception. Would the idea be to do something like:
public class ArgumentOutOfRangeExceptionThrowsAdvice : IThrowsAdvice
{
public ArgumentOutOfRangeExceptionThrowsAdvice()
{
}

public void AfterThrowing(MethodInfo m, object[] args, object target,
DataAccessException ex)
{
if (ex.InnerException is ArgumentOutOfRangeException)
{
this.AfterThrowing(m, args, target, ex.InnerException as ArgumentOutOfRangeException);
}
}

public void AfterThrowing(MethodInfo m, object[] args, object target,
ArgumentOutOfRangeException ex)
{
// do something
}

}
I'm not sure that this is the most elegant approach, but it's something that immediately jumped to mind. What other possibilities are there? Perhaps, there is an adjustment I might make in the overall design?

I think that this issue is something that will come up in many other situations, not just when there are users of IBatis. IBatis isn't the first library I've used that wraps up the root of an exception to help simplify the user experience a bit.

Best regards,

Steve

[Aleks Seovic]

Yeah, I would probably implement it the same way you described -- by checking InnerException manually for this particular case and delegating to appropriate handler.

While maybe not the most elegant approach, I believe that it is better than trying to stick similar logic into the framework itself. This way knowledge of how exceptions are thrown and handled remains in the application, where in my opinion it should be. Framework just gives you a facility to intercept thrown exception -- what you are going to do with it afterwards is entirely up to you.

Regards,

Aleks