Commons Logging
  1. Commons Logging
  2. LOGGING-144

LogFactory/LogFactoryImpl ingore Throwable

    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

        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.
        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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development