Wicket
  1. Wicket
  2. WICKET-2979

Handle Throwable instead of RuntimeException in RequestCycle.step()

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 1.4.9
    • Fix Version/s: None
    • Component/s: wicket
    • Labels:
      None

      Description

      RequestCycle.java:1346 catches only RuntimeException, thus AssertionError and similar are not caught. In case of an AJAX request, the container will respond with an error page which the user will never see.

        Activity

        Hide
        Martin Grigorov added a comment -

        Catching *Error is never a good thing.
        Better use "throw Invalid(State|Argument)Exception" instead of "assert something".
        Disabling assertions in production is also a better idea.

        Show
        Martin Grigorov added a comment - Catching *Error is never a good thing. Better use "throw Invalid(State|Argument)Exception" instead of "assert something". Disabling assertions in production is also a better idea.
        Hide
        Florian Wunderlich added a comment -

        Of course, what was meant was Throwable, not Error.

        Show
        Florian Wunderlich added a comment - Of course, what was meant was Throwable, not Error.
        Hide
        Martin Grigorov added a comment -

        Even worse

        Show
        Martin Grigorov added a comment - Even worse
        Hide
        Florian Wunderlich added a comment -

        Give reasons. No blanket statements.

        What do you want to accomplish with catching RuntimeException?

        If it is generic error handling and reporting, Throwable must be caught. See sun.awt.exception.handler for example.

        Show
        Florian Wunderlich added a comment - Give reasons. No blanket statements. What do you want to accomplish with catching RuntimeException? If it is generic error handling and reporting, Throwable must be caught. See sun.awt.exception.handler for example.
        Hide
        Martin Grigorov added a comment -

        It is not good to catch OutOfMemoryError or ThreadDeathError.
        Check Error.java javadoc:
        An <code>Error</code> is a subclass of <code>Throwable</code>

        • that indicates serious problems that a reasonable application
        • should not try to catch. Most such errors are abnormal conditions.
        • ....
        Show
        Martin Grigorov added a comment - It is not good to catch OutOfMemoryError or ThreadDeathError. Check Error.java javadoc: An <code>Error</code> is a subclass of <code>Throwable</code> that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions. ....
        Hide
        Florian Wunderlich added a comment -

        You might want to check the given example, sun.awt.exception.handler, which is about as authoritative as it gets concerning Java UI toolkits, or ponder the question who else should handle such a Throwable if it is not caught by the toolkit, or even who CAN handle such a Throwable, as all methods that could be overridden are declared final.

        Show
        Florian Wunderlich added a comment - You might want to check the given example, sun.awt.exception.handler, which is about as authoritative as it gets concerning Java UI toolkits, or ponder the question who else should handle such a Throwable if it is not caught by the toolkit, or even who CAN handle such a Throwable, as all methods that could be overridden are declared final.
        Hide
        Johan Compagner added a comment -

        why do you want to catch it there?
        do you want to have it in protected void logRuntimeException(RuntimeException e) of the Requestcycle?

        (that will not work anyway with that signature ofcourse)

        Problem with Errors is that some errors are not really errors so i agree sometimes that catching error or throwable is also fine
        For example why is it a ClassNotFoundException and a NoClassDefFoundError ? which is many times the same problem....

        Show
        Johan Compagner added a comment - why do you want to catch it there? do you want to have it in protected void logRuntimeException(RuntimeException e) of the Requestcycle? (that will not work anyway with that signature ofcourse) Problem with Errors is that some errors are not really errors so i agree sometimes that catching error or throwable is also fine For example why is it a ClassNotFoundException and a NoClassDefFoundError ? which is many times the same problem....
        Hide
        Florian Wunderlich added a comment -

        Yes, I believe that handling it in a method like logRuntimeException would be correct.

        The Throwable would be logged as usual, and cannot go unnoticed in a Servlet container logfile.

        And the behavior when an unhandled exception is encountered would be well-defined and not depend on the Servlet container.

        Last not least, the application would actually have a chance to handle such an exception - otherwise, I believe there is no way an application based on an unpachted Wicket library can handle these exceptions.

        Because the same error page should always be displayed to the user, regardless of what Throwable was thrown, IRequestCycleProcessor.respond should also be switched to Throwable.

        Show
        Florian Wunderlich added a comment - Yes, I believe that handling it in a method like logRuntimeException would be correct. The Throwable would be logged as usual, and cannot go unnoticed in a Servlet container logfile. And the behavior when an unhandled exception is encountered would be well-defined and not depend on the Servlet container. Last not least, the application would actually have a chance to handle such an exception - otherwise, I believe there is no way an application based on an unpachted Wicket library can handle these exceptions. Because the same error page should always be displayed to the user, regardless of what Throwable was thrown, IRequestCycleProcessor.respond should also be switched to Throwable.
        Hide
        Johan Compagner added a comment -

        problem is that with things like out of mem or really "dead" end things
        even a page error could be a problem again.
        And besides that catching Throwable is horrible because you cant really rethrow it
        catch(Error error) throw error is possible and i definitely want to rethrow those kinds of things, so they are only there for logging then. not for displaying a error page, if we would to catch error.

        Show
        Johan Compagner added a comment - problem is that with things like out of mem or really "dead" end things even a page error could be a problem again. And besides that catching Throwable is horrible because you cant really rethrow it catch(Error error) throw error is possible and i definitely want to rethrow those kinds of things, so they are only there for logging then. not for displaying a error page, if we would to catch error.
        Hide
        Florian Wunderlich added a comment -

        An error while handling an error is certainly a possibility, but this is already correctly implemented in RequestCycle.step() using the handlingException flag. Handling fatal errors can never be more than a "best effort" implementation, as you have already pointed out.

        My point is that if there is a method that handles fatal errors, it should handle ALL fatal errors, not just RuntimeException.

        For example, StackOverflowError is usually caused by unchecked recursion, certainly a programming mistake. In such a case, I want to log this error just as I log a SQL error from the persistence provider, informing the maintenance crew for the application, and then I want to show the user the usual error page. There is no point in informing the system administrators because the Servlet container reported an unhandled exception, or showing a generic error page for the Servlet container instead of the applications error page.

        AssertionError is pretty much the same - these also occur in production when they are thrown explicitly, which is not uncommon (a grep over the JDK sources for 6u21 shows 293 occurrences).

        Thus, I propose that either RequestCycle.step() is not declared final, or that Throwable is caught and handled pretty much like RuntimeException now, possibly re-throwing it in an override-able method.

        I do understand that you do not want to catch Throwable because you cannot simply re-throw it, but this is also not uncommon (find -name *.java | xargs grep 'catch *( *Throwable' | wc -l over the same JDK 6u21 sources finds 330 occurrences), and there can be just two cases of unchecked exceptions anyway: Error or RuntimeException. Thus, the handling can be exactly like in java.io.ObjectStreamClass.java:2110 for example - test for Error, then RuntimeException - it must be one or the other.

        Show
        Florian Wunderlich added a comment - An error while handling an error is certainly a possibility, but this is already correctly implemented in RequestCycle.step() using the handlingException flag. Handling fatal errors can never be more than a "best effort" implementation, as you have already pointed out. My point is that if there is a method that handles fatal errors, it should handle ALL fatal errors, not just RuntimeException. For example, StackOverflowError is usually caused by unchecked recursion, certainly a programming mistake. In such a case, I want to log this error just as I log a SQL error from the persistence provider, informing the maintenance crew for the application, and then I want to show the user the usual error page. There is no point in informing the system administrators because the Servlet container reported an unhandled exception, or showing a generic error page for the Servlet container instead of the applications error page. AssertionError is pretty much the same - these also occur in production when they are thrown explicitly, which is not uncommon (a grep over the JDK sources for 6u21 shows 293 occurrences). Thus, I propose that either RequestCycle.step() is not declared final, or that Throwable is caught and handled pretty much like RuntimeException now, possibly re-throwing it in an override-able method. I do understand that you do not want to catch Throwable because you cannot simply re-throw it, but this is also not uncommon (find -name *.java | xargs grep 'catch *( *Throwable' | wc -l over the same JDK 6u21 sources finds 330 occurrences), and there can be just two cases of unchecked exceptions anyway: Error or RuntimeException. Thus, the handling can be exactly like in java.io.ObjectStreamClass.java:2110 for example - test for Error, then RuntimeException - it must be one or the other.
        Hide
        Florian Wunderlich added a comment -

        I think a few important points have been raised since this bug has been closed that could improve error handling in Wicket. If there is still consensus that neither of the proposed changes shall be made, please close it again.

        Show
        Florian Wunderlich added a comment - I think a few important points have been raised since this bug has been closed that could improve error handling in Wicket. If there is still consensus that neither of the proposed changes shall be made, please close it again.
        Hide
        Florian Wunderlich added a comment -

        Change Priority from the default to "Minor".

        Show
        Florian Wunderlich added a comment - Change Priority from the default to "Minor".
        Hide
        Martin Grigorov added a comment -

        I still think this is wrong.
        I'll let someone else to give opinion/decide.

        Show
        Martin Grigorov added a comment - I still think this is wrong. I'll let someone else to give opinion/decide.
        Hide
        Martin Grigorov added a comment -

        The code is reworked in 1.5+.
        Now we catch Exception instead of RuntimeException in IRequestCycleListener#onException() but still not Error/Throwable.
        1.5 years later no one else wanted to catch Throwable ...

        Show
        Martin Grigorov added a comment - The code is reworked in 1.5+. Now we catch Exception instead of RuntimeException in IRequestCycleListener#onException() but still not Error/Throwable. 1.5 years later no one else wanted to catch Throwable ...

          People

          • Assignee:
            Unassigned
            Reporter:
            Florian Wunderlich
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development