The "// Extra aggressive rootCause finding" in StandardWrapper.java is an understatement. It causes an infinite loop in certain cases. For example, the semantics of the following exception class are that getRootCause() returns the one and only root cause of the exception chain, which in this case can be the exception itself. It is not derived from ServletException, thus is not bound to return null at the end of the chain as ServletException does. Its not safe to use reflection to call an arbitrary method of an arbitrary Exception class, just because it happens to have the same name as the method in ServletException. public class MyException extends Exception { public MyException(Throwable cause) { this.cause = cause; } Throwable cause; public Throwable getCause() { return cause; } public Throwable getRootCause() { if (cause instanceof VCOMException) { return ((VCOMException) exception).getRootCause(); } return cause == null ? this : cause; } }
Suggested patch?
(In reply to comment #1) > Suggested patch? Break out of the loop if cause == exception.
The problem is user code: return cause == null ? this : cause; If the root cause is null, then it should return null, not itself.
(In reply to comment #3) > The problem is user code: > return cause == null ? this : cause; > > If the root cause is null, then it should return null, not itself. You can't control every exception class that could be thrown in user code. This is getRootCause(), not getCause(). Lots of java code was written before Sun added getCause() to Throwable and standardized exception chaining. Tomcat calling getRootCause() of an arbitrary exception class is dangerous, as the infinite loop demonstrates. It should either only call it on descendants of ServletException and a few other known types, or at a minimum protect against the infinite loop condition.
At best - here is the patch if any (not me) is interested ... /container/catalina/src/share/org/apache/catalina/core/StandardWrapper.java @@ -627,13 +627,14 @@ /** * Extract the root cause from a servlet exception. - * + * * @param e The servlet exception */ public static Throwable getRootCause(ServletException e) { Throwable rootCause = e; Throwable rootCauseCheck = null; // Extra aggressive rootCause finding + int recursionKlugeDetector = 20; do { try { rootCauseCheck = (Throwable)IntrospectionUtils.getProperty @@ -644,6 +645,16 @@ } catch (ClassCastException ex) { rootCauseCheck = null; } + + /* + We've done this 20 times .. if you've nested more than + 20 levels of exceptions. Tomcat throwing the wrong exception + is the least of your concerns. + */ + + if (recursionKlugeDetector-- == 0) { + return rootCause; + } } while (rootCauseCheck != null); return rootCause; }
see http://issues.apache.org/bugzilla/show_bug.cgi?id=37038 Perhaps Tomcat should use the commons-lang ExceptionUtils root cause finding that already fixes this problem... It appears to be superior to the // Extra aggressive rootCause finding remification.
This issue has caused a production incident for us. It was rather tough to debug given the fact that one would not expect that a getRootCause() method in a non-Tomcat exception actually implements a (hidden) Tomcat interface. IMHO this is very bad style. A recursion detector does not address these issues: 1. Tomcat has no business calling a getRootCause() method in user code. That method may have a very different contract than what Tomcat expects. It may have side effects or it may be illegal to call the method in some cases. Because it is not clear to a user that his getRootCause() method is called by Tomcat and because exception paths are usually not tested that well, production incidents are likely to still happen to some users. 2. The user still does not know that he is implementing a hidden Tomcat interface. 3. 20 recursions (or any arbitrary number) of reflection, with calls to a method of unknown specifications may cause performance issues. I believe that a similar fix to the one for bug 37038 is the best solution. That fix would result in code that does not surprise Java developers, without the problems outlined above. It is also unlikely that it would result in breakage, since I don't think that it's likely that anyone took advantage of this 'feature' to implement some functionality.
Wouter, unfortunately nothing ever gets fixed in Tomcat. The people who can make the changes think there shit doesn't stink, and unless they find the bug and it affects them, it doesn't exist. The patch for this would be like 1 line of code, and the one Tim provided above (with the typical Tomcat developer sarcastic asshole mentality) is almost worse than the current code. I won't write a patch, and you shouldn't either. Noone will incorporate one into the Tomcat codebase, ever, or let you put it in. They will give excuses like, "if we fix it, it could break code that depends on it being broken". Good luck.
it is for a called game Bots
I have fixed the infinite loop issue in svn by breaking out of the loop as Jonathan suggested in comment 2. The fix will be in 5.5.21 onwards. There are certainly more comprehensive ways of doing this, the commons-lang approach is just one of them. If someone would like to provide a patch then I would be happy to review it.
Mark, Thanks for working on this issue. Your change still results in potentially dangerous invocations of getRootCause on third-party classes, though. This fix does not have that flaw and is much cleaner overall: public static Throwable getRootCause(ServletException e) { Throwable rootCause = e.getRootCause(); if (rootCause instanceof ServletException && rootCause != e) { rootCause = getRootCause((ServletException) rootCause); } return rootCause; } Could you please review it?
Thanks for the suggested patch. Whilst it is safer, it also less likely to get at the true root cause and I'd like to keep the behaviour of getting at the root cause wherever possible. Using commons-lang is one option but I'd rather not add yet another jar to the build process. It should be possible to add the ExceptionUtils class to o.a.c.util and use that. It looks like there might be some dependencies on other commons-lang classes that would need to be coded around. If you want to take a look at this - that would be great. If not, no problem and I'll look at it later this week.
Mark, If you really want the root cause, you should do this: public static Throwable getRootCause(ServletException e) { Throwable rootCause = e.getRootCause(); if (rootCause instanceof ServletException && rootCause != e) { rootCause = getRootCause((ServletException) rootCause); } else { Throwable deepRootCause; do { deepRootCause = rootCause.getCause(); if (rootCause == deepRootCause) { return rootCause; } else if (deepRootCause != null) { rootCause = deepRootCause; } } while (deepRootCause != null); } return rootCause; } This code is actually far more likely to find the true root cause and as an added bonus, it uses API's whose implementation you should be able to trust.
That won't work since JspException does not extend ServletException and JspException is typically the reason we wish to delve deeper into the real root cause. This issue is fixed in tomcat6 since getCause() and getRootCause() are imlpemented as one in the same (since 1.5 jdk is required) but since tomcat needs to be able to run on java 1.3 (shudders) - the getCause may not be available for 1.3 implementations. Remarking as fixed for now lness a better patch arrives.
Here is some code that works for both ServletException and JspException: public static Throwable getRootCause(ServletException e) { Throwable rootCause = e.getRootCause(); return processRootCause(e, rootCause); } private static Throwable processRootCause(Exception e, Throwable rootCause) { if (rootCause == null || rootCause == e) { return e; //The root cause is a ServletException or JspException } if (rootCause instanceof ServletException) { rootCause = getRootCause((ServletException) rootCause); } else if (rootCause instanceof JspException) { JspException jspRootCause = (JspException)rootCause; Throwable deeperRootCause = jspRootCause.getRootCause(); rootCause = processRootCause(jspRootCause, deeperRootCause); } return rootCause; //The root cause is not a ServletException or JspException }
Thanks for the new patch. I have committed a variation to svn and will be in 5.5.21 onwards.