Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.1
    • Fix Version/s: 1.1.2
    • Labels:
      None

      Description

      The code in LogFactory/LogFactoryImpl catches and ignores Throwable in several places.

      This is a bad idea, as some Throwables (e.g. ThreadDeath) should never be ignored.

      1. LOGGING-144.patch
        7 kB
        Thomas Neidhart

        Activity

        Sebb created issue -
        Hide
        Thomas Neidhart added a comment -

        Ok, but what should happen in such a case?

        Show
        Thomas Neidhart added a comment - Ok, but what should happen in such a case?
        Hide
        Sebb added a comment -

        Either don't catch everything, or rethrow those throwables that should not be caught.

        Show
        Sebb added a comment - Either don't catch everything, or rethrow those throwables that should not be caught.
        Hide
        Thomas Neidhart added a comment -

        First bunch of changes, please review.

        Missing is the catch in LogFactoryImpl.createLogFromClass which calls then handleFlawedDiscovery.

        Show
        Thomas Neidhart added a comment - First bunch of changes, please review. Missing is the catch in LogFactoryImpl.createLogFromClass which calls then handleFlawedDiscovery.
        Thomas Neidhart made changes -
        Field Original Value New Value
        Attachment LOGGING-144.patch [ 12565482 ]
        Hide
        Sebb added a comment -

        The patch changes the code to how it should have been written originally.

        However, it means that the code may potentially throw some additional unchecked exceptions, whereas previously the code would log them and continue.

        If that is not acceptable, then the alternative approach is to use a method such as

        org.apache.tomcat.util.ExceptionUtils.handleThrowable(Throwable t) [1]

        For example:

        } catch (Throwable t) {
            handleThrowable(t); // may not return
            logDiagnostic(...);
        }
        

        [1] http://svn.apache.org/repos/asf/tomcat/trunk/java/org/apache/tomcat/util/ExceptionUtils.java

        Show
        Sebb added a comment - The patch changes the code to how it should have been written originally. However, it means that the code may potentially throw some additional unchecked exceptions, whereas previously the code would log them and continue. If that is not acceptable, then the alternative approach is to use a method such as org.apache.tomcat.util.ExceptionUtils.handleThrowable(Throwable t) [1] For example: } catch (Throwable t) { handleThrowable(t); // may not return logDiagnostic(...); } [1] http://svn.apache.org/repos/asf/tomcat/trunk/java/org/apache/tomcat/util/ExceptionUtils.java
        Hide
        Thomas Neidhart added a comment -

        Thanks for the pointer, this looks cleaner compared to my patch.

        Although, if I understand it correctly, this will also throw additional unchecked exceptions (ThreadDeath & VirtualMachineError) that where swallowed and logged before?

        Show
        Thomas Neidhart added a comment - Thanks for the pointer, this looks cleaner compared to my patch. Although, if I understand it correctly, this will also throw additional unchecked exceptions (ThreadDeath & VirtualMachineError) that where swallowed and logged before?
        Hide
        Sebb added a comment - - edited

        this will also throw additional unchecked exceptions (ThreadDeath & VirtualMachineError) that where swallowed and logged before?

        Well, yes, but those exceptions should never have been swallowed.

        Show
        Sebb added a comment - - edited this will also throw additional unchecked exceptions (ThreadDeath & VirtualMachineError) that where swallowed and logged before? Well, yes, but those exceptions should never have been swallowed.
        Hide
        Thomas Neidhart added a comment -

        Ok fine, I will then update the patch with your suggestion.

        Thanks again!

        Show
        Thomas Neidhart added a comment - Ok fine, I will then update the patch with your suggestion. Thanks again!
        Hide
        Thomas Neidhart added a comment -

        Done in r1449064.

        Show
        Thomas Neidhart added a comment - Done in r1449064.
        Thomas Neidhart made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 1.1.2 [ 12314498 ]
        Resolution Fixed [ 1 ]
        Thomas Neidhart made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        219d 24m 1 Thomas Neidhart 22/Feb/13 14:49
        Resolved Resolved Closed Closed
        26d 5h 21m 1 Thomas Neidhart 20/Mar/13 20:11

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development