Commons Logging
  1. Commons Logging
  2. LOGGING-98

[logging][PATCH] Improvements to LogFactoryImpl

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      Attached is a draft version of a proposed rework of the LogFactoryImpl discovery
      mechanism. This work is based on recent discussions on the commons-dev mailing
      list.

      The prime difference between the proposal and the existing class is that when
      the new discovery process "discovers" a potential Log implementation, it
      immediately attempts to create a Log instance. Only if an instance is
      successfully created is the implementation consider "discovered." The existing
      LogFactoryImpl considers an Log implementation to be discovered if it can load
      its class, but if there are any subsequent problems actually instantiating an
      object, the discovery process is already over and the code has no choice but to
      throw an exception.

      This proposed approach allows implementation of a couple of ideas Robert Burrell
      Donkin proposed on the dev list:

      1) If discovery is able to load an implementation class using the TCCL, but
      instantiation fails, discovery continues with an attempt to load and instantiate
      the class using LogFactoryImpl's classloader.

      2) If an implementation cannot be instantiated, no exception is thrown, but
      rather discovery continues to try other implementations, beginning with
      java.util.logging.

      In addition, the proposed code will write diagnostic messages to System.out and
      System.err if a Log class can be loaded but not instantiated. This is to help
      users understand why their desired Log implementation was not used. Some users
      may prefer to have JCL throw an exception. How this is handled could be made
      configurable; the proposed code at this point does not include this feature but
      it could be added.

      The intent of the attachment is to promote discussion/seek review, and since its
      a pretty significant refactor I've attached a complete file rather than a diff.
      To ease comparison the file includes a fair amount of commented out code from
      the prior version.

      Any comments or suggestions would be most appreciated.

      1. ASF.LICENSE.NOT.GRANTED--LogFactoryImplD.diff
        3 kB
        Brian Stansberry
      2. ASF.LICENSE.NOT.GRANTED--LogFactoryImplC.diff
        5 kB
        Brian Stansberry
      3. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl8c.diff
        6 kB
        Brian Stansberry
      4. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl7c.diff
        2 kB
        Brian Stansberry
      5. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl6a.diff
        1.0 kB
        Brian Stansberry
      6. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl5b.diff
        6 kB
        Brian Stansberry
      7. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl5a.diff
        33 kB
        Brian Stansberry
      8. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl4b.diff
        6 kB
        Brian Stansberry
      9. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl4a.diff
        32 kB
        Brian Stansberry
      10. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl4.diff
        38 kB
        Brian Stansberry
      11. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl.java
        25 kB
        Brian Stansberry
      12. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl.java
        22 kB
        Brian Stansberry
      13. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl.java
        39 kB
        Brian Stansberry
      14. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl.diff
        15 kB
        Brian Stansberry
      15. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl.diff
        32 kB
        Brian Stansberry
      16. ASF.LICENSE.NOT.GRANTED--LogFactoryImpl.diff
        6 kB
        Brian Stansberry
      17. ASF.LICENSE.NOT.GRANTED--LoadTest.diff
        1 kB
        Brian Stansberry

        Activity

        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=14866)
        LogFactoryImpl.java

        Show
        Brian Stansberry added a comment - Created an attachment (id=14866) LogFactoryImpl.java
        Hide
        rdonkin@apache.org added a comment -

        Thanks for the patch

        Setting asside the obvious minor issues (those commented out protected methods
        need to be added back in for backwards compatibility), my first impressions are
        positive. Need more time to really go through the code in detail, though, it'll
        probably be Sunday before I get a chance. Adding a tidied up diff (no comments)
        would also be useful (it would be good to have both): a diff would help me to
        track down all the changes and understand them more easily.

        Might even create some documentation describing the control flow in each case.

        Robert

        Show
        rdonkin@apache.org added a comment - Thanks for the patch Setting asside the obvious minor issues (those commented out protected methods need to be added back in for backwards compatibility), my first impressions are positive. Need more time to really go through the code in detail, though, it'll probably be Sunday before I get a chance. Adding a tidied up diff (no comments) would also be useful (it would be good to have both): a diff would help me to track down all the changes and understand them more easily. Might even create some documentation describing the control flow in each case. Robert
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=14880)
        diff to LogFactoryImpl

        OK, had a chance to clean up some; attached is a diff. The code could still
        stand to simmer on the stove a while; I particularly want to check through the
        exception handling.

        Thanks for pointing out the protected methods; they've been uncommented. I
        changed their implementation to be more consistent with the way the new
        discovery works. This means they will behave differently than the current
        implementation. I could have left the old code (in which case they would be
        completely backwards compatible). Not sure what the right approach is.

        For now I've left all the new methods I wrote private; some could certainly be
        protected if we decide we want to expose them.

        I'll attach a new copy of the class (non-diff) in a moment.

        If I get a chance I'll try to come up with something to doc the code flow, but
        might be a while.

        Show
        Brian Stansberry added a comment - Created an attachment (id=14880) diff to LogFactoryImpl OK, had a chance to clean up some; attached is a diff. The code could still stand to simmer on the stove a while; I particularly want to check through the exception handling. Thanks for pointing out the protected methods; they've been uncommented. I changed their implementation to be more consistent with the way the new discovery works. This means they will behave differently than the current implementation. I could have left the old code (in which case they would be completely backwards compatible). Not sure what the right approach is. For now I've left all the new methods I wrote private; some could certainly be protected if we decide we want to expose them. I'll attach a new copy of the class (non-diff) in a moment. If I get a chance I'll try to come up with something to doc the code flow, but might be a while.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=14881)
        LogFactoryImpl.java rev2

        source file to match previous diff

        Show
        Brian Stansberry added a comment - Created an attachment (id=14881) LogFactoryImpl.java rev2 source file to match previous diff
        Hide
        Brian Stansberry added a comment -

        Neglected to add in last comment: Using the attached code, the LoadTest test
        case fails at line 114. I believe this is because the test case expects failure
        if the TCCL is the system classloader. Need to look further into this.

        Show
        Brian Stansberry added a comment - Neglected to add in last comment: Using the attached code, the LoadTest test case fails at line 114. I believe this is because the test case expects failure if the TCCL is the system classloader. Need to look further into this.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15207)
        diff to LogFactoryImpl rev 3

        Attached is a considerably cleaned up and revised version of the previous
        patches. Major changes are:

        1) Incorporates the internal diagnostics Simon Kitching recently added to SVN
        trunk.
        2) Adds configuration property "org.apache.commons.logging.FailOnException",
        configurable either as a system property or in a commons-logging.properties
        file. If set to true, JCL will fail with a LogConfigurationException if a Log
        adapter and its associated logging library are discovered but an instance
        cannot be created. This situation is referred to as a "flawed discovery". If
        the property is false or unset, flawed discoveries will be logged using
        whatever log adapter is eventually discovered.
        3) Lots and lots of comments.
        4) General tightening up of the code, fixing of subtle issues, etc.
        5) Protected methods that are no longer invoked by the class are marked as
        deprecated (wouldn't mind chopping them out – this class now exceeds 1000
        lines).

        Using this class all the unit tests invoked by the build.xml test target pass.
        There is a problem with the LoadTest unit test found in the src/test tree; I've
        posted on commons-dev about this.

        Using this class all 32 cases listed in Robert's demonstration document work as
        expected. To see examples of how the logging of "flawed discovery" works, run
        cases 20, 24 and 28.

        Need to test what happens if I turn on the "FailOnException" switch, but
        thought it would be good to let others have a look.

        Will follow with an attachment of the complete source file in case its easier
        to read.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15207) diff to LogFactoryImpl rev 3 Attached is a considerably cleaned up and revised version of the previous patches. Major changes are: 1) Incorporates the internal diagnostics Simon Kitching recently added to SVN trunk. 2) Adds configuration property "org.apache.commons.logging.FailOnException", configurable either as a system property or in a commons-logging.properties file. If set to true, JCL will fail with a LogConfigurationException if a Log adapter and its associated logging library are discovered but an instance cannot be created. This situation is referred to as a "flawed discovery". If the property is false or unset, flawed discoveries will be logged using whatever log adapter is eventually discovered. 3) Lots and lots of comments. 4) General tightening up of the code, fixing of subtle issues, etc. 5) Protected methods that are no longer invoked by the class are marked as deprecated (wouldn't mind chopping them out – this class now exceeds 1000 lines). Using this class all the unit tests invoked by the build.xml test target pass. There is a problem with the LoadTest unit test found in the src/test tree; I've posted on commons-dev about this. Using this class all 32 cases listed in Robert's demonstration document work as expected. To see examples of how the logging of "flawed discovery" works, run cases 20, 24 and 28. Need to test what happens if I turn on the "FailOnException" switch, but thought it would be good to let others have a look. Will follow with an attachment of the complete source file in case its easier to read.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15208)
        LogFactoryImpl.java rev 3

        full java file to match diff I just attached

        Show
        Brian Stansberry added a comment - Created an attachment (id=15208) LogFactoryImpl.java rev 3 full java file to match diff I just attached
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15210)
        diff to LogFactoryImpl rev 3a

        Beginning a series of 4 progressive patches that subdivide the changes between
        the previously attached Rev 3 and the version in trunk.

        The attached is a patch to trunk that incorporates the basic idea of requiring
        the discovery process to actually instantiate a Log object before considering
        an adapter to be discovered.

        All tests called by the test target in build.xml pass with this version.
        LoadTest also passes (which I regard as a negative – means discovery can't
        handle a misconfigured TCCL). Robert's demonstration tests pass except Nos.
        20, 24, 28 and 32 which fail with an LCE.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15210) diff to LogFactoryImpl rev 3a Beginning a series of 4 progressive patches that subdivide the changes between the previously attached Rev 3 and the version in trunk. The attached is a patch to trunk that incorporates the basic idea of requiring the discovery process to actually instantiate a Log object before considering an adapter to be discovered. All tests called by the test target in build.xml pass with this version. LoadTest also passes (which I regard as a negative – means discovery can't handle a misconfigured TCCL). Robert's demonstration tests pass except Nos. 20, 24, 28 and 32 which fail with an LCE.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15211)
        diff to LogFactoryImpl rev 3b

        This is a patch to the file that would result from applying the previous 3a.

        Adds the feature whereby if an adapter and associated logger are loaded via the
        TCCL but log instantiation fails, an attempt is made to reload the classes
        using LogFactoryImpl's classloader.

        With this patch, all unit tests in build.xml pass. LoadTest fails, which I
        regard as a positive, as it shows the patch allows LogFactoryImpl to handle a
        misconfigured TCCL. Results on Robert's demonstration tests are the same as
        for 3a, except for some reason case 24 succeeds. This deserves more attention.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15211) diff to LogFactoryImpl rev 3b This is a patch to the file that would result from applying the previous 3a. Adds the feature whereby if an adapter and associated logger are loaded via the TCCL but log instantiation fails, an attempt is made to reload the classes using LogFactoryImpl's classloader. With this patch, all unit tests in build.xml pass. LoadTest fails, which I regard as a positive, as it shows the patch allows LogFactoryImpl to handle a misconfigured TCCL. Results on Robert's demonstration tests are the same as for 3a, except for some reason case 24 succeeds. This deserves more attention.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15212)
        diff to LogFactoryImpl rev 3c

        This is a patch to the file that would result from applying the previous 3a and
        3b.

        Adds the ability to continue discovery following a "flawed discovery" (i.e.
        adapter and logger can be loaded but instantiation fails). Provides a
        configuration property to control whether this feature is enabled. It is by
        default.

        Passes all unit tests in build.xml. LoadTest fails, which I regard as a
        postive. All tests in Robert's demonstration setup pass.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15212) diff to LogFactoryImpl rev 3c This is a patch to the file that would result from applying the previous 3a and 3b. Adds the ability to continue discovery following a "flawed discovery" (i.e. adapter and logger can be loaded but instantiation fails). Provides a configuration property to control whether this feature is enabled. It is by default. Passes all unit tests in build.xml. LoadTest fails, which I regard as a postive. All tests in Robert's demonstration setup pass.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15213)
        diff to LogFactoryImpl rev 3d

        This is a patch to the file that would be created by applying 3a-c.

        Adds a feature whereby if there is a "flawed discovery" and JCL isn't
        configured to immediately fail, the Throwable that represents the "flaw" is
        cached. If another Log implementation is subsequently instantiated, that
        implementation class is used to log the cached throwable as a warning. This
        provides greater visibility to the end user.

        All units tests in build.xml pass. LoadTest fails, which I regard as a
        positive. All of Robert's demonstration cases pass, and for cases 20, 28 and 32
        the above described WARN message is recorded.

        Good night!

        Show
        Brian Stansberry added a comment - Created an attachment (id=15213) diff to LogFactoryImpl rev 3d This is a patch to the file that would be created by applying 3a-c. Adds a feature whereby if there is a "flawed discovery" and JCL isn't configured to immediately fail, the Throwable that represents the "flaw" is cached. If another Log implementation is subsequently instantiated, that implementation class is used to log the cached throwable as a warning. This provides greater visibility to the end user. All units tests in build.xml pass. LoadTest fails, which I regard as a positive. All of Robert's demonstration cases pass, and for cases 20, 28 and 32 the above described WARN message is recorded. Good night!
        Hide
        Simon Kitching added a comment -

        re patch 3a:

        In discoverLogImplementation, when the user has specified an explicit log class
        then we shouldn't do any discovery, yes? We should just load that logging class
        or fail. But I'm not sure that is what the code is doing. If
        handleFlawedDiscovery returns, then the flow drops down into the discovery part,
        right? So maybe the code needs a throw new LogConfiguratioException after the
        last handleFlawedDiscovery? And then the log4j discovery doesn't need to be in
        an else, as that if-statement never exits out the bottom.

        When testing for the jdk14 logger, I'm not sure that the code to check that java
        1.4 is actually available belongs here. I'm surprised that it would be possible
        to instantiate a Jdk14Logger in a pre-1.4 JVM at all, as the class does have
        parameters with types only available in java1.4. However if java's "lazy
        linking" is so advanced that it doesn't even resolve those types until needed,
        then we can simply fix that by having a static block in Jdk14Logger that
        references Level.class or similar. It seems to me to be cleaner for Logger
        classes to be responsible for validating whether they can run or not than
        putting this in the LogFactoryImpl class.

        I think there's a copy-and-paste error in the comments for the Lumberjack test

        Method createLogFromClass has side-effects: it sets a number of instance
        variables as well as returning the Log object. But the isXXXAvailable methods
        also call createLogFromClass - meaning that if anyone called one of those and it
        succeeded, then this would "reset" the logConstructor and related members.
        Possibly createLogFromClass could be split into two parts, with only the
        side-effect-free part being called from the isXXXAvailable methods, but I would
        prefer to simply do away with the isXXXAvailable methods completely.

        The comments on handleFlawedDiscovery still reference #FAILURE_PROPERTY, though
        I presume the actual code is in a different patch.

        Show
        Simon Kitching added a comment - re patch 3a: In discoverLogImplementation, when the user has specified an explicit log class then we shouldn't do any discovery, yes? We should just load that logging class or fail. But I'm not sure that is what the code is doing. If handleFlawedDiscovery returns, then the flow drops down into the discovery part, right? So maybe the code needs a throw new LogConfiguratioException after the last handleFlawedDiscovery? And then the log4j discovery doesn't need to be in an else, as that if-statement never exits out the bottom. When testing for the jdk14 logger, I'm not sure that the code to check that java 1.4 is actually available belongs here. I'm surprised that it would be possible to instantiate a Jdk14Logger in a pre-1.4 JVM at all, as the class does have parameters with types only available in java1.4. However if java's "lazy linking" is so advanced that it doesn't even resolve those types until needed, then we can simply fix that by having a static block in Jdk14Logger that references Level.class or similar. It seems to me to be cleaner for Logger classes to be responsible for validating whether they can run or not than putting this in the LogFactoryImpl class. I think there's a copy-and-paste error in the comments for the Lumberjack test Method createLogFromClass has side-effects: it sets a number of instance variables as well as returning the Log object. But the isXXXAvailable methods also call createLogFromClass - meaning that if anyone called one of those and it succeeded, then this would "reset" the logConstructor and related members. Possibly createLogFromClass could be split into two parts, with only the side-effect-free part being called from the isXXXAvailable methods, but I would prefer to simply do away with the isXXXAvailable methods completely. The comments on handleFlawedDiscovery still reference #FAILURE_PROPERTY, though I presume the actual code is in a different patch.
        Hide
        Brian Stansberry added a comment -

        (In reply to comment #12)
        > re patch 3a:
        >
        > In discoverLogImplementation, when the user has specified an explicit log class
        > then we shouldn't do any discovery, yes? We should just load that logging class
        > or fail. But I'm not sure that is what the code is doing. If
        > handleFlawedDiscovery returns, then the flow drops down into the discovery part,
        > right?

        Arggh. Good catch. In this version handleFlawedDiscovery always throws a
        LogConfigurationException, but still discoverLogImplementation shouldn't count
        on that fact.

        >
        > When testing for the jdk14 logger, I'm not sure that the code to check that java
        > 1.4 is actually available belongs here. I'm surprised that it would be possible
        > to instantiate a Jdk14Logger in a pre-1.4 JVM at all, as the class does have
        > parameters with types only available in java1.4. However if java's "lazy
        > linking" is so advanced that it doesn't even resolve those types until needed,
        > then we can simply fix that by having a static block in Jdk14Logger that
        > references Level.class or similar. It seems to me to be cleaner for Logger
        > classes to be responsible for validating whether they can run or not than
        > putting this in the LogFactoryImpl class.
        >
        +1.

        I don't like that stuff in here either, and your static initializer idea removes
        any remote justification for it.

        > I think there's a copy-and-paste error in the comments for the Lumberjack test
        >

        ???

        > Method createLogFromClass has side-effects: it sets a number of instance
        > variables as well as returning the Log object. But the isXXXAvailable methods
        > also call createLogFromClass - meaning that if anyone called one of those and it
        > succeeded, then this would "reset" the logConstructor and related members.
        > Possibly createLogFromClass could be split into two parts, with only the
        > side-effect-free part being called from the isXXXAvailable methods, but I would
        > prefer to simply do away with the isXXXAvailable methods completely.
        >

        Haven't thought carefully but this could perhaps be handled with a simple "if
        (logConstructor == null)" test around the code that sets instance variables. I
        too would prefer to see the isXXXAvailable methods go, even more so now that
        you've pointed out the can have side effects.

        > The comments on handleFlawedDiscovery still reference #FAILURE_PROPERTY, though
        > I presume the actual code is in a different patch.

        Yes. Will fix.

        Thanks for the careful review.

        Show
        Brian Stansberry added a comment - (In reply to comment #12) > re patch 3a: > > In discoverLogImplementation, when the user has specified an explicit log class > then we shouldn't do any discovery, yes? We should just load that logging class > or fail. But I'm not sure that is what the code is doing. If > handleFlawedDiscovery returns, then the flow drops down into the discovery part, > right? Arggh. Good catch. In this version handleFlawedDiscovery always throws a LogConfigurationException, but still discoverLogImplementation shouldn't count on that fact. > > When testing for the jdk14 logger, I'm not sure that the code to check that java > 1.4 is actually available belongs here. I'm surprised that it would be possible > to instantiate a Jdk14Logger in a pre-1.4 JVM at all, as the class does have > parameters with types only available in java1.4. However if java's "lazy > linking" is so advanced that it doesn't even resolve those types until needed, > then we can simply fix that by having a static block in Jdk14Logger that > references Level.class or similar. It seems to me to be cleaner for Logger > classes to be responsible for validating whether they can run or not than > putting this in the LogFactoryImpl class. > +1. I don't like that stuff in here either, and your static initializer idea removes any remote justification for it. > I think there's a copy-and-paste error in the comments for the Lumberjack test > ??? > Method createLogFromClass has side-effects: it sets a number of instance > variables as well as returning the Log object. But the isXXXAvailable methods > also call createLogFromClass - meaning that if anyone called one of those and it > succeeded, then this would "reset" the logConstructor and related members. > Possibly createLogFromClass could be split into two parts, with only the > side-effect-free part being called from the isXXXAvailable methods, but I would > prefer to simply do away with the isXXXAvailable methods completely. > Haven't thought carefully but this could perhaps be handled with a simple "if (logConstructor == null)" test around the code that sets instance variables. I too would prefer to see the isXXXAvailable methods go, even more so now that you've pointed out the can have side effects. > The comments on handleFlawedDiscovery still reference #FAILURE_PROPERTY, though > I presume the actual code is in a different patch. Yes. Will fix. Thanks for the careful review.
        Hide
        Brian Stansberry added a comment -

        Please ignore previous comment about "if (logConstructor == null)" test; that
        only works properly if isXXXAvailable was called after getInstance.

        Passing a boolean arg to createLogFromClass could be a simple fix, but it smells
        bad. Let's try to reach a consensus on whether the unused protected methods
        should remain.

        Show
        Brian Stansberry added a comment - Please ignore previous comment about "if (logConstructor == null)" test; that only works properly if isXXXAvailable was called after getInstance. Passing a boolean arg to createLogFromClass could be a simple fix, but it smells bad. Let's try to reach a consensus on whether the unused protected methods should remain.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15221)
        diff to LogFactoryImpl rev 4a

        Replaces previous rev 3a addressing the issues discussed above.

        The "else" clause around the attempt to load log4j remains for now, since if we
        ever get to diff c it will have to go back in. If not we can take it out.
        Starting to get a bit nuts keeping these correct, so want to change as little
        as possible between versions.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15221) diff to LogFactoryImpl rev 4a Replaces previous rev 3a addressing the issues discussed above. The "else" clause around the attempt to load log4j remains for now, since if we ever get to diff c it will have to go back in. If not we can take it out. Starting to get a bit nuts keeping these correct, so want to change as little as possible between versions.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15222)
        diff to LogFactoryImpl rev 4b

        Replacement for 3b reflecting comments above.

        I won't generate a 4c until everyone has had a chance to comment on these.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15222) diff to LogFactoryImpl rev 4b Replacement for 3b reflecting comments above. I won't generate a 4c until everyone has had a chance to comment on these.
        Hide
        Simon Kitching added a comment -

        I still don't understand that bit with the else around the log4j discovery.

        If someone has specified an explicit logclass, ie
        if (specifiedLogClassName != null)
        is true then surely:

        • we create an instance of the specified class and return it, or
        • we fail with an exception

        I don't think we should ever fall back to discovery mode if
        specifiedLogClassName is not null. So I would write this:

        if (specifiedLogClassName != null) {
        try

        { // note: createLogClass never returns null.. result = createLogClass(...); return result; }

        catch(LogConfigurationException ex)

        { // this type of exception means we've alread output diagnostics // about this issue, etc.; just pass it on throw ex; }

        catch(Throwable t)

        { // log problem, and potentially throw an exception if the user // doesn't want recovery from flawed discovery handleFlawedDiscovery(..); // even if configuration info states that we should recover from // flawed logging classes, we *never* recover from a flawed log // which was explicitly specified by the user... throw new LogConfigurationException(t); }

        // this if-statement never exits!
        }

        // test for log4j
        try {
        result = .....

        Show
        Simon Kitching added a comment - I still don't understand that bit with the else around the log4j discovery. If someone has specified an explicit logclass, ie if (specifiedLogClassName != null) is true then surely: we create an instance of the specified class and return it, or we fail with an exception I don't think we should ever fall back to discovery mode if specifiedLogClassName is not null. So I would write this: if (specifiedLogClassName != null) { try { // note: createLogClass never returns null.. result = createLogClass(...); return result; } catch(LogConfigurationException ex) { // this type of exception means we've alread output diagnostics // about this issue, etc.; just pass it on throw ex; } catch(Throwable t) { // log problem, and potentially throw an exception if the user // doesn't want recovery from flawed discovery handleFlawedDiscovery(..); // even if configuration info states that we should recover from // flawed logging classes, we *never* recover from a flawed log // which was explicitly specified by the user... throw new LogConfigurationException(t); } // this if-statement never exits! } // test for log4j try { result = .....
        Hide
        Simon Kitching added a comment -

        (In reply to comment #17)

        PS: and this code would remain the same whatever future changes are made to
        createLogClass or handleFlawedDiscovery...

        Show
        Simon Kitching added a comment - (In reply to comment #17) PS: and this code would remain the same whatever future changes are made to createLogClass or handleFlawedDiscovery...
        Hide
        Brian Stansberry added a comment -

        (In reply to comment #18)
        > (In reply to comment #17)
        >
        > PS: and this code would remain the same whatever future changes are made to
        > createLogClass or handleFlawedDiscovery...
        >

        You're right. I was resisting the approach of returning after the call to
        createLogClass() because I generally don't like a bunch of return statements in
        a method. But I realize now I was thinking in terms of returning after every
        call to createLogClass(), which would add a bunch of returns. This is not
        necessary and not what you're suggesting; adding a return for the one special
        case of specifiedLogClassName makes sense and clarifies the code.

        Thanks.

        Show
        Brian Stansberry added a comment - (In reply to comment #18) > (In reply to comment #17) > > PS: and this code would remain the same whatever future changes are made to > createLogClass or handleFlawedDiscovery... > You're right. I was resisting the approach of returning after the call to createLogClass() because I generally don't like a bunch of return statements in a method. But I realize now I was thinking in terms of returning after every call to createLogClass(), which would add a bunch of returns. This is not necessary and not what you're suggesting; adding a return for the one special case of specifiedLogClassName makes sense and clarifies the code. Thanks.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15238)
        diff to LogFactoryImpl 5a

        Updated to reflect comments above. Some of the code comments differ from what
        you wrote, Simon, as this patch does not yet include the option to recover from
        flawed discovery.

        Also replaced the rest of the silly "if (t instanceof
        LogConfigurationException)" stuff with normal catch blocks. Couple of comment
        fixes too.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15238) diff to LogFactoryImpl 5a Updated to reflect comments above. Some of the code comments differ from what you wrote, Simon, as this patch does not yet include the option to recover from flawed discovery. Also replaced the rest of the silly "if (t instanceof LogConfigurationException)" stuff with normal catch blocks. Couple of comment fixes too.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15259)
        diff to LogFactoryImpl rev 5b

        Patch to LogFactoryImpl in trunk, following recent commit of patch 5a – thanks
        Simon

        Description of the patch is as per comment #9 above.

        Had a chance to think about why case Robert's 24 succeeds with this patch
        applied, and it is as expected. Case 20 and 24 both have the caller in the
        parent loader with the TCCL set to the child. In both cases JCL is in both the
        parent and child. In 20 Log4j is in the child; in 24 it is in both parent and
        child. Initial discovery fails in both cases because the Log discovered by the
        TCCL is incompatible with the LogFactory bound to the caller. Case 24 succeeds
        with this patch because when an attempt is made to load Log4jLogger using the
        parent loader, log4j.jar is visible to the parent loader, so discovery
        succeeds.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15259) diff to LogFactoryImpl rev 5b Patch to LogFactoryImpl in trunk, following recent commit of patch 5a – thanks Simon Description of the patch is as per comment #9 above. Had a chance to think about why case Robert's 24 succeeds with this patch applied, and it is as expected. Case 20 and 24 both have the caller in the parent loader with the TCCL set to the child. In both cases JCL is in both the parent and child. In 20 Log4j is in the child; in 24 it is in both parent and child. Initial discovery fails in both cases because the Log discovered by the TCCL is incompatible with the LogFactory bound to the caller. Case 24 succeeds with this patch because when an attempt is made to load Log4jLogger using the parent loader, log4j.jar is visible to the parent loader, so discovery succeeds.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15260)
        diff to LoadTest

        If patch 5b is applied, LoadTest will fail. As discussed on the dev list, it's
        not part of the standard test suite, but in the interest of not having a test
        that fails in the codebase, attached is a simple fix that recognizes
        LogFactoryImpl should not throw an exception with a misconfigured TCCL.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15260) diff to LoadTest If patch 5b is applied, LoadTest will fail. As discussed on the dev list, it's not part of the standard test suite, but in the interest of not having a test that fails in the codebase, attached is a simple fix that recognizes LogFactoryImpl should not throw an exception with a misconfigured TCCL.
        Hide
        Simon Kitching added a comment -

        I'm opposed to attempting to load a specific adapter from the parent classpath
        if one is found in the child classpath which doesn't work. If the user has the
        lib in the child classpath, then:

        • there's no guarantee it is the same version of the lib as in the parent, and
        • the one in the parent will behave quite differently anyway, as it wont see
          config files in the child etc.

        I think this situation should be regarded as a failed discovery, which is
        exactly what the existing code does. Invisibly failing over to the lib in the
        parent could cause the user major confusion.

        Show
        Simon Kitching added a comment - I'm opposed to attempting to load a specific adapter from the parent classpath if one is found in the child classpath which doesn't work. If the user has the lib in the child classpath, then: there's no guarantee it is the same version of the lib as in the parent, and the one in the parent will behave quite differently anyway, as it wont see config files in the child etc. I think this situation should be regarded as a failed discovery , which is exactly what the existing code does. Invisibly failing over to the lib in the parent could cause the user major confusion.
        Hide
        Simon Kitching added a comment -

        Hi Brian,

        As part of your patch to add a configurable "ignore flawed discovery" feature,
        could you please consider handing the cast to Log in this line:
        logAdapter = (Log) constructor.newInstance(params);
        separately from the other exceptions?

        Currently, the code catches all throwables and passes the exception down to
        handleFlawedDiscovery, which then assumes that a ClassCastException was due to
        casting the result to Log. This isn't necessarily true. I think it would be nice
        if the return value from constructor.newInstance was initially treated as an
        Object. Casting could then be done in a separate try/catch clause, and only
        ClassCastException in that block would mean calling reportInvalidLogAdapter.

        Anyway, I think it's worth considering...and as I know you're already working on
        these methods it didn't seem wise for me to be working on the same code at the
        same time .

        PS: When affectState is false, do wewant to call handleFlawedDiscovery? I
        suspect not..

        Show
        Simon Kitching added a comment - Hi Brian, As part of your patch to add a configurable "ignore flawed discovery" feature, could you please consider handing the cast to Log in this line: logAdapter = (Log) constructor.newInstance(params); separately from the other exceptions? Currently, the code catches all throwables and passes the exception down to handleFlawedDiscovery, which then assumes that a ClassCastException was due to casting the result to Log. This isn't necessarily true. I think it would be nice if the return value from constructor.newInstance was initially treated as an Object. Casting could then be done in a separate try/catch clause, and only ClassCastException in that block would mean calling reportInvalidLogAdapter. Anyway, I think it's worth considering...and as I know you're already working on these methods it didn't seem wise for me to be working on the same code at the same time . PS: When affectState is false, do wewant to call handleFlawedDiscovery? I suspect not..
        Hide
        Simon Kitching added a comment -

        (In reply to comment #23)
        > I'm opposed to attempting to load a specific adapter from the parent classpath
        > if one is found in the child classpath which doesn't work.

        Sorry, I got confused. This is only going to happen when getLog is called from
        code in the parent classpath. And there just isn't any other solution anyway.

        I'll have a look at the patch (#21) now.

        Show
        Simon Kitching added a comment - (In reply to comment #23) > I'm opposed to attempting to load a specific adapter from the parent classpath > if one is found in the child classpath which doesn't work. Sorry, I got confused. This is only going to happen when getLog is called from code in the parent classpath. And there just isn't any other solution anyway. I'll have a look at the patch (#21) now.
        Hide
        Brian Stansberry added a comment -

        (In reply to comment #25)
        > (In reply to comment #23)
        > > I'm opposed to attempting to load a specific adapter from the parent
        classpath
        > > if one is found in the child classpath which doesn't work.
        >
        > Sorry, I got confused. This is only going to happen when getLog is called from
        > code in the parent classpath. And there just isn't any other solution anyway.
        >
        > I'll have a look at the patch (#21) now.
        >

        Actually, I'm glad you raised this point, as it got me thinking how to respond,
        and I thought of a couple issues. Was thinking in shower and car, so may be off
        a bit, but:

        1) This logic should only be invoked in cases of ClassCastException, not
        InvocationTargetException, etc.
        2) What if there is a commons-logging.properties in the child? There will be
        some strange interactions here:
        a) If the Log adapter specified by commmons-logging.properties CAN be loaded by
        the parent, it will be, even if that's not what would be normally discovered at
        the parent level.
        b) If the Log adapter specified by commmons-logging.properties CANNOT be loaded
        by the parent, JCL will fail, as we do not allow normal discovery to proceed.

        Show
        Brian Stansberry added a comment - (In reply to comment #25) > (In reply to comment #23) > > I'm opposed to attempting to load a specific adapter from the parent classpath > > if one is found in the child classpath which doesn't work. > > Sorry, I got confused. This is only going to happen when getLog is called from > code in the parent classpath. And there just isn't any other solution anyway. > > I'll have a look at the patch (#21) now. > Actually, I'm glad you raised this point, as it got me thinking how to respond, and I thought of a couple issues. Was thinking in shower and car, so may be off a bit, but: 1) This logic should only be invoked in cases of ClassCastException, not InvocationTargetException, etc. 2) What if there is a commons-logging.properties in the child? There will be some strange interactions here: a) If the Log adapter specified by commmons-logging.properties CAN be loaded by the parent, it will be, even if that's not what would be normally discovered at the parent level. b) If the Log adapter specified by commmons-logging.properties CANNOT be loaded by the parent, JCL will fail, as we do not allow normal discovery to proceed.
        Hide
        Simon Kitching added a comment -

        (In reply to comment #26)
        >
        > 1) This logic should only be invoked in cases of ClassCastException, not
        > InvocationTargetException, etc.

        Actually, only when ClassCastException happens when trying to convert to Log..

        (2a) This sounds ok to me. The user wanted that logging lib, and they didn't
        specify FailOnHierarchyError (or whatever we call it)
        (2b) This sounds ok to me. The user didn't want discovery..

        On related matters, I would prefer not to take the approach:

        • try context
        • try classloader of the LogFactoryImpl.

        I think it would be better to start at the context classloader and walk up the
        classloader inheritance tree until reaching the classloader that loaded the
        LogFactoryImpl. I've been playing with various ways of achieving this, but it
        isn't easy when you also need to treat exceptions as fatal or recoverable along
        the way. Opinions?

        Show
        Simon Kitching added a comment - (In reply to comment #26) > > 1) This logic should only be invoked in cases of ClassCastException, not > InvocationTargetException, etc. Actually, only when ClassCastException happens when trying to convert to Log.. (2a) This sounds ok to me. The user wanted that logging lib, and they didn't specify FailOnHierarchyError (or whatever we call it) (2b) This sounds ok to me. The user didn't want discovery.. On related matters, I would prefer not to take the approach: try context try classloader of the LogFactoryImpl. I think it would be better to start at the context classloader and walk up the classloader inheritance tree until reaching the classloader that loaded the LogFactoryImpl. I've been playing with various ways of achieving this, but it isn't easy when you also need to treat exceptions as fatal or recoverable along the way. Opinions?
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15301)
        diff to LogFactoryImpl 6a

        Restore handling of ClassNotFoundException to discovery process. This got
        inadvertently dropped in the code cleanup, and without it Robert's
        demonstration cases 9, 10, 13, 14 and 26 fail.

        BTW Robert, huge kudos to you on your effort setting up the demonstration
        harness. Using it is making this work so much easier, both in terms of finding
        problems and in understanding theoretically why some things work and some
        don't.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15301) diff to LogFactoryImpl 6a Restore handling of ClassNotFoundException to discovery process. This got inadvertently dropped in the code cleanup, and without it Robert's demonstration cases 9, 10, 13, 14 and 26 fail. BTW Robert, huge kudos to you on your effort setting up the demonstration harness. Using it is making this work so much easier, both in terms of finding problems and in understanding theoretically why some things work and some don't.
        Hide
        Brian Stansberry added a comment -

        (In reply to comment #27)
        > (In reply to comment #26)
        >
        > On related matters, I would prefer not to take the approach:
        > * try context
        > * try classloader of the LogFactoryImpl.
        >
        > I think it would be better to start at the context classloader and walk up the
        > classloader inheritance tree until reaching the classloader that loaded the
        > LogFactoryImpl. I've been playing with various ways of achieving this, but it
        > isn't easy when you also need to treat exceptions as fatal or recoverable along
        > the way. Opinions?
        (In reply to comment #27)
        > (In reply to comment #26)
        >
        > On related matters, I would prefer not to take the approach:
        > * try context
        > * try classloader of the LogFactoryImpl.
        >
        > I think it would be better to start at the context classloader and walk up the
        > classloader inheritance tree until reaching the classloader that loaded the
        > LogFactoryImpl. I've been playing with various ways of achieving this, but it
        > isn't easy when you also need to treat exceptions as fatal or recoverable along
        > the way. Opinions?

        I'm not sure there is a situation where trying any other classloader will work
        better. I think there are four possible relationships between the thead context
        classsloader (hereafter called TCCL) and LogFactoryImpl's classloader (hereafter
        called LFICL).

        1) TCCL is a child of LFICL.

        In this case, no Log adapter class defined by any intermediate classloader
        between TCCL and LFICL will be compatible with LFI. Log adapter must be defined
        by LFICL or a parent of LFICL.

        No logging library class defined by any intermediate classloader between TCCL
        and LFICL will be compatible with any compatible Log adapter. Logging adapter
        must be defined by the log adapter's classloader or a parent of it.

        Therefore, no class defined by any intermediate classloader between TCCL and
        LFICL will be useful. Falling back to discovery using LFICL is the most
        efficient next step if TCCL fails.

        2) TCCL is a parent of LFICL.

        In this situation, the only reason discovery using the TCCL would have failed is
        if LFICL or an intermediate loader between it and TCCL uses child-first loading
        and loaded a different version of Log directly.

        It is possible to walk down the classloader hierarchy from TCCL to LFICL,
        trying to find a classloader that works, but in the end you'll get the same
        result as if you'd just tried LFICL. So, again falling back to discovery using
        LFICL is the most efficient next step if TCCL fails.

        3) TCCL is LFICL.

        Not much we can do here

        4) TCCL is a sibling or cousin of LFICL.

        The only other classloader that could possibly work is a classloader that is a
        common parent of both TCCL and LFICL. But, classes loaded by such a classloader
        will only be compatible if there is no intermediate loader between it and LFICL
        that uses child-first loading and can load a different version of Log directly.
        Trying to find such a "common-parent" classloader is possible, but in the end
        you'll get the same result as if you'd just tried LFICL. So, again falling back
        to discovery using LFICL is the most efficient next step if TCCL fails.

        I believe most of the above is outside the scope of what you'd mentioned, but
        thought it might be useful to cover all the cases.

        Show
        Brian Stansberry added a comment - (In reply to comment #27) > (In reply to comment #26) > > On related matters, I would prefer not to take the approach: > * try context > * try classloader of the LogFactoryImpl. > > I think it would be better to start at the context classloader and walk up the > classloader inheritance tree until reaching the classloader that loaded the > LogFactoryImpl. I've been playing with various ways of achieving this, but it > isn't easy when you also need to treat exceptions as fatal or recoverable along > the way. Opinions? (In reply to comment #27) > (In reply to comment #26) > > On related matters, I would prefer not to take the approach: > * try context > * try classloader of the LogFactoryImpl. > > I think it would be better to start at the context classloader and walk up the > classloader inheritance tree until reaching the classloader that loaded the > LogFactoryImpl. I've been playing with various ways of achieving this, but it > isn't easy when you also need to treat exceptions as fatal or recoverable along > the way. Opinions? I'm not sure there is a situation where trying any other classloader will work better. I think there are four possible relationships between the thead context classsloader (hereafter called TCCL) and LogFactoryImpl's classloader (hereafter called LFICL). 1) TCCL is a child of LFICL. In this case, no Log adapter class defined by any intermediate classloader between TCCL and LFICL will be compatible with LFI. Log adapter must be defined by LFICL or a parent of LFICL. No logging library class defined by any intermediate classloader between TCCL and LFICL will be compatible with any compatible Log adapter. Logging adapter must be defined by the log adapter's classloader or a parent of it. Therefore, no class defined by any intermediate classloader between TCCL and LFICL will be useful. Falling back to discovery using LFICL is the most efficient next step if TCCL fails. 2) TCCL is a parent of LFICL. In this situation, the only reason discovery using the TCCL would have failed is if LFICL or an intermediate loader between it and TCCL uses child-first loading and loaded a different version of Log directly. It is possible to walk down the classloader hierarchy from TCCL to LFICL, trying to find a classloader that works, but in the end you'll get the same result as if you'd just tried LFICL. So, again falling back to discovery using LFICL is the most efficient next step if TCCL fails. 3) TCCL is LFICL. Not much we can do here 4) TCCL is a sibling or cousin of LFICL. The only other classloader that could possibly work is a classloader that is a common parent of both TCCL and LFICL. But, classes loaded by such a classloader will only be compatible if there is no intermediate loader between it and LFICL that uses child-first loading and can load a different version of Log directly. Trying to find such a "common-parent" classloader is possible, but in the end you'll get the same result as if you'd just tried LFICL. So, again falling back to discovery using LFICL is the most efficient next step if TCCL fails. I believe most of the above is outside the scope of what you'd mentioned, but thought it might be useful to cover all the cases.
        Hide
        Simon Kitching added a comment -

        Thanks for patch 6a - committed.

        Re scenario (1) of comment#29: Consider tomcat's classloader hierarchy:
        http://jakarta.apache.org/tomcat/tomcat-5.5-doc/class-loader-howto.html

        Log/LogFactory/LogFactoryImpl is deployed at the "system" level (this is
        standard out-of-the-box configuration for tomcat).

        Now if someone deploys JCL at the "shared" level and at the shared level
        deploys Log4JLog+log4j but not Log, then you have a situation where:

        • TCCL fails (Log duplicated)
        • parent of TCCL succeeds (and is not LFICL).
        Show
        Simon Kitching added a comment - Thanks for patch 6a - committed. Re scenario (1) of comment#29: Consider tomcat's classloader hierarchy: http://jakarta.apache.org/tomcat/tomcat-5.5-doc/class-loader-howto.html Log/LogFactory/LogFactoryImpl is deployed at the "system" level (this is standard out-of-the-box configuration for tomcat). Now if someone deploys JCL at the "shared" level and at the shared level deploys Log4JLog+log4j but not Log, then you have a situation where: TCCL fails (Log duplicated) parent of TCCL succeeds (and is not LFICL).
        Hide
        Simon Kitching added a comment -

        (In reply to comment #30)
        oops - small but critical typo. I'll try again...

        Re scenario (1) of comment#29: Consider tomcat's classloader hierarchy:
        http://jakarta.apache.org/tomcat/tomcat-5.5-doc/class-loader-howto.html

        Log/LogFactory/LogFactoryImpl is deployed at the "system" level (this is
        standard out-of-the-box configuration for tomcat).

        Now if someone deploys JCL at the "webapp" level and at the shared or common
        level deploys Log4JLog+log4j but not Log, then you have a situation where:

        • TCCL [webapp] fails (Log duplicated)
        • parent of TCCL [shared] succeeds (and is not LFICL).
        Show
        Simon Kitching added a comment - (In reply to comment #30) oops - small but critical typo. I'll try again... Re scenario (1) of comment#29: Consider tomcat's classloader hierarchy: http://jakarta.apache.org/tomcat/tomcat-5.5-doc/class-loader-howto.html Log/LogFactory/LogFactoryImpl is deployed at the "system" level (this is standard out-of-the-box configuration for tomcat). Now if someone deploys JCL at the "webapp" level and at the shared or common level deploys Log4JLog+log4j but not Log, then you have a situation where: TCCL [webapp] fails (Log duplicated) parent of TCCL [shared] succeeds (and is not LFICL).
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15377)
        patch to allowFlawed/.../LogFactoryImpl 7c

        (Attached patch is to the LogFactoryImpl now on the "allowFlawed' branch.)

        1) Invocation of constructor in createLogFromClass catches
        InvocationTargetException and rethrows its target. This handles the situation
        where underlying problems we wish to ignore (e.g. NoClassDefFoundError) are not
        surfaced until we invoke the constructor. In some experimentation I'm doing I
        found that this situation occurs with LogKitLogger.

        2) Main try/catch block in createLogFromClass specifically catches and rethrows
        LCE. Otherwise LCE thrown by handleFlawedHierarchy will needlessly be wrapped
        by another LCE in the final "catch Throwable" block.

        Show
        Brian Stansberry added a comment - Created an attachment (id=15377) patch to allowFlawed/.../LogFactoryImpl 7c (Attached patch is to the LogFactoryImpl now on the "allowFlawed' branch.) 1) Invocation of constructor in createLogFromClass catches InvocationTargetException and rethrows its target. This handles the situation where underlying problems we wish to ignore (e.g. NoClassDefFoundError) are not surfaced until we invoke the constructor. In some experimentation I'm doing I found that this situation occurs with LogKitLogger. 2) Main try/catch block in createLogFromClass specifically catches and rethrows LCE. Otherwise LCE thrown by handleFlawedHierarchy will needlessly be wrapped by another LCE in the final "catch Throwable" block.
        Hide
        Brian Stansberry added a comment -

        Created an attachment (id=15378)
        patch to allowFlawed/.../LogFactoryImpl 8c

        (Attached is a patch to the file that would result from applying the previous
        "7c" patch to the LogFactoryImpl now on the "allowFlawed' branch.)

        I was concerned that there might be pretty significant differences in JCL
        behavior depending on whether a user included a commons-logging.properties
        file, so I played with the demonstration harness to see what would happen.
        Basically here's what I did:

        – Added a parallel test case for each of the 32 existing cases.
        – In each parallel case, replaced any usage of log4j.jar with logkit.jar.
        – In each parallel case, added to the child loader classpath a
        commons-logging.properties file specifying logkit.

        I then recorded the differences in results between the parallel cases. There
        were 3 kinds of differences:

        1) (Cases 5, 6, 17, 21, 22, 25, 29) Here the original case discovered log4j
        and the parallel case discovered JDK logging. This is simply because in these
        cases the TCCL is the system classloader, so LogFactory cannot find
        commons-logging.properties. Within the scope of what I'm testing here (cases
        where code deployed in a child loader is trying to configure JCL), I don't
        regard this as a "fixable" problem.

        2) (Cases 3 and 4) Here the original case discovers JDK logging but in the
        parallel case JCL fails with an LCE. This is because in these cases
        LogKitLogger is defined by parent, logkit.jar by child, so LogKitLogger cannot
        resolve logkit.jar. The LCE seems fairly legitimate to me, particularly in
        case 3 where the caller is defined by the child loader. The user stated the log
        library they want, which can't be loaded. This can easily be remedied by
        adding logkit.jar to the parent classpath as cases 7 and 8 show.

        3) (Cases 20, 28 and 32) Again the original case discovers JDK logging but in
        the parallel case JCL fails with an LCE. But here the problem is the
        incompatible Log interface problem. In these cases, I don't think it is valid
        for JCL to always throw an LCE. If the user wants an LCE thrown, they can
        set the ALLOW_FLAWED_HIERARCHY_PROPERTY to false. The current behavior
        penalizes users who want to run logkit, avalon, etc. by leaving them no way to
        avoid the LCE.

        I can think of 3 approaches to handling the 2nd and 3rd category problems. The
        first is simply to strip out the throwing of an LCE from
        discoverLogImplementation – just let normal discovery proceed if the user
        specified class can't be instantiated. This approach prevents LCEs in the 2nd
        and 3rd category cases. However, it doesn't allow failure when it might be
        appropriate.

        The attached patch takes the 2nd approach. It has handleFlawedDiscovery and
        handleFlawedHierarchy set a flag if they are invoked but are configured to
        ignore the flaw. If this flag is set, discoverLogImplementation knows the
        specified adapter was loaded but there was a problem the user wants to ignore.
        In that case, it allows normal discovery to proceed. This approach only
        resolves the 3rd category problems. Cases 3 and 4 still fail with LCEs.

        A 3rd approach would build on the attached patch. It would add another
        ALLOW_...._PROPERTY letting the user specify what they want to happen in cases
        3 and 4.

        Any thoughts?

        Show
        Brian Stansberry added a comment - Created an attachment (id=15378) patch to allowFlawed/.../LogFactoryImpl 8c (Attached is a patch to the file that would result from applying the previous "7c" patch to the LogFactoryImpl now on the "allowFlawed' branch.) I was concerned that there might be pretty significant differences in JCL behavior depending on whether a user included a commons-logging.properties file, so I played with the demonstration harness to see what would happen. Basically here's what I did: – Added a parallel test case for each of the 32 existing cases. – In each parallel case, replaced any usage of log4j.jar with logkit.jar. – In each parallel case, added to the child loader classpath a commons-logging.properties file specifying logkit. I then recorded the differences in results between the parallel cases. There were 3 kinds of differences: 1) (Cases 5, 6, 17, 21, 22, 25, 29) Here the original case discovered log4j and the parallel case discovered JDK logging. This is simply because in these cases the TCCL is the system classloader, so LogFactory cannot find commons-logging.properties. Within the scope of what I'm testing here (cases where code deployed in a child loader is trying to configure JCL), I don't regard this as a "fixable" problem. 2) (Cases 3 and 4) Here the original case discovers JDK logging but in the parallel case JCL fails with an LCE. This is because in these cases LogKitLogger is defined by parent, logkit.jar by child, so LogKitLogger cannot resolve logkit.jar. The LCE seems fairly legitimate to me, particularly in case 3 where the caller is defined by the child loader. The user stated the log library they want, which can't be loaded. This can easily be remedied by adding logkit.jar to the parent classpath as cases 7 and 8 show. 3) (Cases 20, 28 and 32) Again the original case discovers JDK logging but in the parallel case JCL fails with an LCE. But here the problem is the incompatible Log interface problem. In these cases, I don't think it is valid for JCL to always throw an LCE. If the user wants an LCE thrown, they can set the ALLOW_FLAWED_HIERARCHY_PROPERTY to false. The current behavior penalizes users who want to run logkit, avalon, etc. by leaving them no way to avoid the LCE. I can think of 3 approaches to handling the 2nd and 3rd category problems. The first is simply to strip out the throwing of an LCE from discoverLogImplementation – just let normal discovery proceed if the user specified class can't be instantiated. This approach prevents LCEs in the 2nd and 3rd category cases. However, it doesn't allow failure when it might be appropriate. The attached patch takes the 2nd approach. It has handleFlawedDiscovery and handleFlawedHierarchy set a flag if they are invoked but are configured to ignore the flaw. If this flag is set, discoverLogImplementation knows the specified adapter was loaded but there was a problem the user wants to ignore. In that case, it allows normal discovery to proceed. This approach only resolves the 3rd category problems. Cases 3 and 4 still fail with LCEs. A 3rd approach would build on the attached patch. It would add another ALLOW_...._PROPERTY letting the user specify what they want to happen in cases 3 and 4. Any thoughts?
        Hide
        Simon Kitching added a comment -

        (In reply to comment #32)
        > 1) Invocation of constructor in createLogFromClass

        I think this should be fixed in LogKitLogger rather than in LogFactoryImpl.

        The issue is telling the difference between a logging implementation *not being
        available* and the logging implementation being available but broken. When the
        logging impl is not available, discovery should continue. But I think that when
        it is available but broken we should throw an exception rather than mysteriously
        redirecting output to another logging implementation.

        It's difficult to tell these apart, but I would suggest specifying (in the Log
        interface javadoc) that a NoClassDefFound or an ExceptionInInitializerError
        should indicate "not available" while an InvocationTargetException indicates
        available-but-broken. This is the approach that I took with Jdk14Logger, where
        there is a dummy variable that forces an ExceptionInInitializerError if
        java.util.logging.Level is not available. I expect a similar thing could be done
        with LogKitLogger...

        Thoughts?

        >
        > 2) Main try/catch block in createLogFromClass specifically catches and rethrows
        > LCE. Otherwise LCE thrown by handleFlawedHierarchy will needlessly be wrapped
        > by another LCE in the final "catch Throwable" block.

        Well spotted. I'll commit this ASAP

        Show
        Simon Kitching added a comment - (In reply to comment #32) > 1) Invocation of constructor in createLogFromClass I think this should be fixed in LogKitLogger rather than in LogFactoryImpl. The issue is telling the difference between a logging implementation *not being available* and the logging implementation being available but broken. When the logging impl is not available, discovery should continue. But I think that when it is available but broken we should throw an exception rather than mysteriously redirecting output to another logging implementation. It's difficult to tell these apart, but I would suggest specifying (in the Log interface javadoc) that a NoClassDefFound or an ExceptionInInitializerError should indicate "not available" while an InvocationTargetException indicates available-but-broken. This is the approach that I took with Jdk14Logger, where there is a dummy variable that forces an ExceptionInInitializerError if java.util.logging.Level is not available. I expect a similar thing could be done with LogKitLogger... Thoughts? > > 2) Main try/catch block in createLogFromClass specifically catches and rethrows > LCE. Otherwise LCE thrown by handleFlawedHierarchy will needlessly be wrapped > by another LCE in the final "catch Throwable" block. Well spotted. I'll commit this ASAP
        Hide
        Brian Stansberry added a comment -

        (In reply to comment #33)
        <snip>
        >
        > I then recorded the differences in results between the parallel cases. There
        > were 3 kinds of differences:
        >
        > 1) (Cases 5, 6, 17, 21, 22, 25, 29) Here the original case discovered log4j
        > and the parallel case discovered JDK logging. This is simply because in these
        > cases the TCCL is the system classloader, so LogFactory cannot find
        > commons-logging.properties. Within the scope of what I'm testing here (cases
        > where code deployed in a child loader is trying to configure JCL), I don't
        > regard this as a "fixable" problem.

        I wanted to follow up on this a bit, to see what would happen if
        commons-logging.properties were at the parent level instead of the child.
        (Rather than changing the code, I cheated a bit and had the run target in the
        ant file set a JVM property; this should have the same effect, but if I'm
        thinking wrong please let me know).

        I tested against a class that reflects my recent patches. 11 cases (Nos. 1-4,
        9, 10, 13, 14, 18, 26 and 30) fail with an LCE. In all these cases, in Robert's
        original version JDK logging is discovered.

        These results tell me that users who are forced to set the LOG_PROPERTY to get a
        library other than log4j or jdk discovered will have significantly different
        behavior from those who can just put log4j.jar on the classpath. I don't think
        such a big difference is right; I'm thinking the 3rd approach mentioned above is
        the way to go.

        BTW, in getConfigurationValue() there is a cut-and-paste bug in the system
        property check where the LOG_PROPERTY constant is checked instead of the
        property passed to the method. I'd submit a patch, but don't want to confuse
        things with the other two patches just submitted.

        Show
        Brian Stansberry added a comment - (In reply to comment #33) <snip> > > I then recorded the differences in results between the parallel cases. There > were 3 kinds of differences: > > 1) (Cases 5, 6, 17, 21, 22, 25, 29) Here the original case discovered log4j > and the parallel case discovered JDK logging. This is simply because in these > cases the TCCL is the system classloader, so LogFactory cannot find > commons-logging.properties. Within the scope of what I'm testing here (cases > where code deployed in a child loader is trying to configure JCL), I don't > regard this as a "fixable" problem. I wanted to follow up on this a bit, to see what would happen if commons-logging.properties were at the parent level instead of the child. (Rather than changing the code, I cheated a bit and had the run target in the ant file set a JVM property; this should have the same effect, but if I'm thinking wrong please let me know). I tested against a class that reflects my recent patches. 11 cases (Nos. 1-4, 9, 10, 13, 14, 18, 26 and 30) fail with an LCE. In all these cases, in Robert's original version JDK logging is discovered. These results tell me that users who are forced to set the LOG_PROPERTY to get a library other than log4j or jdk discovered will have significantly different behavior from those who can just put log4j.jar on the classpath. I don't think such a big difference is right; I'm thinking the 3rd approach mentioned above is the way to go. BTW, in getConfigurationValue() there is a cut-and-paste bug in the system property check where the LOG_PROPERTY constant is checked instead of the property passed to the method. I'd submit a patch, but don't want to confuse things with the other two patches just submitted.
        Hide
        Brian Stansberry added a comment -

        (In reply to comment #34)
        > (In reply to comment #32)
        > > 1) Invocation of constructor in createLogFromClass
        >
        > I think this should be fixed in LogKitLogger rather than in LogFactoryImpl.
        >
        <snip>
        >
        > It's difficult to tell these apart, but I would suggest specifying (in the Log
        > interface javadoc) that a NoClassDefFound or an ExceptionInInitializerError
        > should indicate "not available" while an InvocationTargetException indicates
        > available-but-broken. This is the approach that I took with Jdk14Logger, where
        > there is a dummy variable that forces an ExceptionInInitializerError if
        > java.util.logging.Level is not available. I expect a similar thing could be done
        > with LogKitLogger...
        >
        > Thoughts?

        +1. I was about 65% of that opinion when I added that big comment in the patch

        Show
        Brian Stansberry added a comment - (In reply to comment #34) > (In reply to comment #32) > > 1) Invocation of constructor in createLogFromClass > > I think this should be fixed in LogKitLogger rather than in LogFactoryImpl. > <snip> > > It's difficult to tell these apart, but I would suggest specifying (in the Log > interface javadoc) that a NoClassDefFound or an ExceptionInInitializerError > should indicate "not available" while an InvocationTargetException indicates > available-but-broken. This is the approach that I took with Jdk14Logger, where > there is a dummy variable that forces an ExceptionInInitializerError if > java.util.logging.Level is not available. I expect a similar thing could be done > with LogKitLogger... > > Thoughts? +1. I was about 65% of that opinion when I added that big comment in the patch
        Hide
        Simon Kitching added a comment -

        Re the InvocationTargetException for LogKitLogger:

        Does it actually matter whether ExceptionInInitializerError or
        InvocationTargetException occurs for LogKitLogger? It does for logadapters that
        are part of auto-discovery as we want to continue discovery in the former case
        but not the latter. But LogKitLogger is not part of auto-discovery, and
        obviously no out-of-tree logger will be. So the only way LogFactoryImpl will
        ever try these loggers is when specificLogClassName != null - but in that case,
        we throw an LCE regardless of whether ExceptionInInitializerError or
        InvocationTargetException occurred, yes?

        And all the auto-discovered loggers should now be working as expected. So I
        think (hope) things are ok as they are..

        Re comment #33 point 3:

        Yep, I pretty much agree with your analysis. However there is another option:
        just choose not to support this, and make the users fix their problem properly.
        Most of the problems reported against JCL are by users who are trying to use JCL
        "out of the box" and are confused it doesn't work. And I sympathise with them -
        they don't want to use JCL, it just came with a library and they just want it
        to shut up and not interfere with them running their app.

        I think people who are explicitly specifying logadapters via
        commons-logging.properties files are generally likely to be able to fix their
        classpath problems. And the next release of JCL should include a separate
        adapters-only jar which is the correct solution to this problem.

        So I'm inclined to just ignore this problem. People who specify a log class will
        be no worse off than JCL 1.0.4 if they throw commons-logging.jar in their
        webapp, and won't have any problems at all if they use
        commons-logging-adapters.jar (or whatever we choose to call it).

        Comments?

        NB: I think it would be a good idea to wind up this bugzilla entry sometime
        soon. It's getting rather clumsy to read/navigate. We could start another entry
        for the next topic - or just move to the mailing list until we need to start
        attaching new patches.

        PS: thanks a lot for your comments/discussion of logging. It's really making me
        think . For such a small project this really is quite difficult isn't it?!

        Show
        Simon Kitching added a comment - Re the InvocationTargetException for LogKitLogger: Does it actually matter whether ExceptionInInitializerError or InvocationTargetException occurs for LogKitLogger? It does for logadapters that are part of auto-discovery as we want to continue discovery in the former case but not the latter. But LogKitLogger is not part of auto-discovery, and obviously no out-of-tree logger will be. So the only way LogFactoryImpl will ever try these loggers is when specificLogClassName != null - but in that case, we throw an LCE regardless of whether ExceptionInInitializerError or InvocationTargetException occurred, yes? And all the auto-discovered loggers should now be working as expected. So I think (hope) things are ok as they are.. Re comment #33 point 3: Yep, I pretty much agree with your analysis. However there is another option: just choose not to support this, and make the users fix their problem properly. Most of the problems reported against JCL are by users who are trying to use JCL "out of the box" and are confused it doesn't work. And I sympathise with them - they don't want to use JCL, it just came with a library and they just want it to shut up and not interfere with them running their app. I think people who are explicitly specifying logadapters via commons-logging.properties files are generally likely to be able to fix their classpath problems. And the next release of JCL should include a separate adapters-only jar which is the correct solution to this problem. So I'm inclined to just ignore this problem. People who specify a log class will be no worse off than JCL 1.0.4 if they throw commons-logging.jar in their webapp, and won't have any problems at all if they use commons-logging-adapters.jar (or whatever we choose to call it). Comments? NB: I think it would be a good idea to wind up this bugzilla entry sometime soon. It's getting rather clumsy to read/navigate. We could start another entry for the next topic - or just move to the mailing list until we need to start attaching new patches. PS: thanks a lot for your comments/discussion of logging. It's really making me think . For such a small project this really is quite difficult isn't it?!
        Hide
        Simon Kitching added a comment -

        All the relevant bits from this ticket have long been merged into
        commons-logging so I'm going to close this.

        Show
        Simon Kitching added a comment - All the relevant bits from this ticket have long been merged into commons-logging so I'm going to close this.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development