Log4j 2
  1. Log4j 2
  2. LOG4J2-230

Preempt StackOverflowEx when both slf4j-impl jar and log4j-to-slf4j jar are on the classpath

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0-beta5
    • Fix Version/s: None
    • Component/s: SLF4J Bridge
    • Labels:
      None

      Description

      This ticket is to prevent issues like LOG4J2-204.

      Aiming for something similar to what is documented here:
      http://www.slf4j.org/codes.html#jclDelegationLoop

        Issue Links

          Activity

          Hide
          Remko Popma added a comment -

          Added this constructor to org.apache.logging.slf4j.SLF4JLoggerContext

          public SLF4JLoggerContext() {
          // LOG4J2-230, LOG4J2-204 (improve error reporting when misconfigured)
          try

          { Class.forName("org.slf4j.helpers.Log4JLoggerFactory"); throw new IllegalStateException("slf4j-impl jar is mutually exclusive with log4j-to-slf4j jar " + "(the first routes calls from SLF4J to Log4j, the second from Log4j to SLF4J)"); }

          catch (Throwable classNotFoundIsGood)

          { // org.slf4j.helpers.Log4JLoggerFactory is not on classpath. Good! }

          }

          Committed in revision 1478048. All existing unit tests still work.

          Show
          Remko Popma added a comment - Added this constructor to org.apache.logging.slf4j.SLF4JLoggerContext public SLF4JLoggerContext() { // LOG4J2-230 , LOG4J2-204 (improve error reporting when misconfigured) try { Class.forName("org.slf4j.helpers.Log4JLoggerFactory"); throw new IllegalStateException("slf4j-impl jar is mutually exclusive with log4j-to-slf4j jar " + "(the first routes calls from SLF4J to Log4j, the second from Log4j to SLF4J)"); } catch (Throwable classNotFoundIsGood) { // org.slf4j.helpers.Log4JLoggerFactory is not on classpath. Good! } } Committed in revision 1478048. All existing unit tests still work.
          Hide
          Nick Williams added a comment -

          There's a major problem in this commit. Do you see it?

          Show
          Nick Williams added a comment - There's a major problem in this commit. Do you see it?
          Hide
          Nick Williams added a comment -

          (Hint: How does the IllegalStateException ever make it past that catch statement?)

          Show
          Nick Williams added a comment - (Hint: How does the IllegalStateException ever make it past that catch statement?)
          Hide
          Remko Popma added a comment -

          Oops. grin

          we independently found the same problem.

          The code is now

          public SLF4JLoggerContextFactory() {
          // LOG4J2-230, LOG4J2-204 (improve error reporting when misconfigured)
          boolean misconfigured = false;
          try

          { Class.forName("org.slf4j.helpers.Log4JLoggerFactory"); misconfigured = true; }

          catch (Throwable classNotFoundIsGood)

          { // org.slf4j.helpers.Log4JLoggerFactory is not on classpath. Good! }

          if (misconfigured)

          { throw new IllegalStateException("slf4j-impl jar is mutually exclusive with log4j-to-slf4j jar " + "(the first routes calls from SLF4J to Log4j, the second from Log4j to SLF4J)"); }

          }

          Committed revision 1478075

          Show
          Remko Popma added a comment - Oops. grin we independently found the same problem. The code is now public SLF4JLoggerContextFactory() { // LOG4J2-230 , LOG4J2-204 (improve error reporting when misconfigured) boolean misconfigured = false; try { Class.forName("org.slf4j.helpers.Log4JLoggerFactory"); misconfigured = true; } catch (Throwable classNotFoundIsGood) { // org.slf4j.helpers.Log4JLoggerFactory is not on classpath. Good! } if (misconfigured) { throw new IllegalStateException("slf4j-impl jar is mutually exclusive with log4j-to-slf4j jar " + "(the first routes calls from SLF4J to Log4j, the second from Log4j to SLF4J)"); } } Committed revision 1478075
          Hide
          Remko Popma added a comment -

          No idea how to create a junit test for this (suggestions, anyone?) but my standalone test now throws the expected error, so I'm marking this issue as fixed.

          Show
          Remko Popma added a comment - No idea how to create a junit test for this (suggestions, anyone?) but my standalone test now throws the expected error, so I'm marking this issue as fixed.
          Hide
          Remko Popma added a comment -

          my test program looks like this:

          import org.slf4j.Logger;
          import org.slf4j.LoggerFactory;

          // at a minimum, set classpath to include:
          // slf4j-api-1.7.2.jar
          // log4j-api-2.0-beta6-SNAPSHOT.jar
          // log4j-slf4j-impl-2.0-beta6-SNAPSHOT.jar
          // log4j-to-slf4j-2.0-beta6-SNAPSHOT.jar
          public class Slf4jTest {
          public static void main(String[] args)

          { Logger logger = LoggerFactory.getLogger(Slf4jTest.class); logger.info("test"); }

          }

          Show
          Remko Popma added a comment - my test program looks like this: import org.slf4j.Logger; import org.slf4j.LoggerFactory; // at a minimum, set classpath to include: // slf4j-api-1.7.2.jar // log4j-api-2.0-beta6-SNAPSHOT.jar // log4j-slf4j-impl-2.0-beta6-SNAPSHOT.jar // log4j-to-slf4j-2.0-beta6-SNAPSHOT.jar public class Slf4jTest { public static void main(String[] args) { Logger logger = LoggerFactory.getLogger(Slf4jTest.class); logger.info("test"); } }
          Hide
          Remko Popma added a comment -

          Nick, thanks for keeping a watchful eye out!

          Show
          Remko Popma added a comment - Nick, thanks for keeping a watchful eye out!
          Hide
          Nick Williams added a comment -

          You're quite welcome. Two points:

          1) IIRC, Class.forName ONLY throws ClassNotFoundException. It NEVER throws anything else (well, technically, any method can throw ThreadDeathException, but one should never stop that from percolating). I should be double-checked, but I think it's safe to JUST catch CNFE, which would allow you to get rid of the local variable and throw the ISE from the try block.

          2) One could argue that writing a unit test for this is more trouble than it's worth. I personally disagree with this, because I think the things that are hardest to test are sometimes the things than need it most, but to each his own. Here is how I would test it. First, I would write a normal unit test that just called the constructor (this tests the "success condition" and fails if the constructor throws an exception). Then I would write the "failure condition" test like so:

          A) Use the compiler tools to compile a simple, empty class named org.slf4j.helpers.Log4JLoggerFactory. Write the resulting class file to %temp%/%unitTestName%/org/slf4j/helpers/Log4JLoggerFactory.class.

          B) Instantiate a java.net.URLClassLoader, using the current Thread's context class loader as the parent loader and passing in a single-element URL array to the %temp%/%unitTestName%/ directory when calling the constructor.

          C) Using reflection and the custom class loader, attempt to load and instantiate the SLF4JLoggerContextFactory. Catch an InvocationTargetException. If the exception is not thrown, fail("Invocation exception not thrown.") the test. If the exception is thrown, get its cause. The cause should be an IllegalStateException. If it is some other exception, fail("Wrong exception type ___ thrown.") the test. Check to make sure the message is the same (just in case). If it's not, fail("Exception message not correct.") the test.

          D) In a finally block, delete the %temp%/%unitTestName% directory.

          Show
          Nick Williams added a comment - You're quite welcome. Two points: 1) IIRC, Class.forName ONLY throws ClassNotFoundException. It NEVER throws anything else (well, technically, any method can throw ThreadDeathException, but one should never stop that from percolating). I should be double-checked, but I think it's safe to JUST catch CNFE, which would allow you to get rid of the local variable and throw the ISE from the try block. 2) One could argue that writing a unit test for this is more trouble than it's worth. I personally disagree with this, because I think the things that are hardest to test are sometimes the things than need it most, but to each his own. Here is how I would test it. First, I would write a normal unit test that just called the constructor (this tests the "success condition" and fails if the constructor throws an exception). Then I would write the "failure condition" test like so: A) Use the compiler tools to compile a simple, empty class named org.slf4j.helpers.Log4JLoggerFactory. Write the resulting class file to %temp%/%unitTestName%/org/slf4j/helpers/Log4JLoggerFactory.class. B) Instantiate a java.net.URLClassLoader, using the current Thread's context class loader as the parent loader and passing in a single-element URL array to the %temp%/%unitTestName%/ directory when calling the constructor. C) Using reflection and the custom class loader, attempt to load and instantiate the SLF4JLoggerContextFactory. Catch an InvocationTargetException. If the exception is not thrown, fail("Invocation exception not thrown.") the test. If the exception is thrown, get its cause. The cause should be an IllegalStateException. If it is some other exception, fail("Wrong exception type ___ thrown.") the test. Check to make sure the message is the same (just in case). If it's not, fail("Exception message not correct.") the test. D) In a finally block, delete the %temp%/%unitTestName% directory.
          Hide
          Remko Popma added a comment -

          Nick,

          about 1)
          I was also worried about NoClassDefFoundError.
          Actually I kind of like having the variable there, makes it more self-documenting.

          2) I was thinking of a different solution but I realized that my idea would only work during the maven build.
          When the JUnit tests for either log4j-to-slf4j or for slf4j-impl are run in isolation, it is not clear where to find the jar file for the other module...
          Your solution solves that problem, thanks!
          Not sure when I'll have the time (no, let's be honest, when I'll want to make the time) to do this...

          Show
          Remko Popma added a comment - Nick, about 1) I was also worried about NoClassDefFoundError. Actually I kind of like having the variable there, makes it more self-documenting. 2) I was thinking of a different solution but I realized that my idea would only work during the maven build. When the JUnit tests for either log4j-to-slf4j or for slf4j-impl are run in isolation, it is not clear where to find the jar file for the other module... Your solution solves that problem, thanks! Not sure when I'll have the time (no, let's be honest, when I'll want to make the time) to do this...
          Hide
          Nick Williams added a comment -

          If NoClassDefFoundError is found here, it should not be caught. It should be allowed to propagate.

          ClassNotFoundException: Thrown when code attempts to load a class via reflection. This is a checked exception, because the code loading the class could not be checked by the compiler, so it must be checked at runtime.

          NoClassDefFoundError: This is thrown when code that WAS checked by the compiler fails to load at runtime. This happens when a class that was successfully linked at compile time cannot be located on the class path. For example:

          public class Animal

          { public Fur fur; }

          If Fur cannot be loaded here, a NoClassDefFoundError is thrown. Fur, in this case, was linked at compile time, but that linkage broke at runtime.

          If Class.forName() throws a NoClassDefFoundError in this case, it could be while loading that class, OR it could be some other major problem with the JRE class path. Importantly, IF NoClassDefFoundError is thrown while loading Log4JLoggerFactory, that means Log4JLoggerFactory WAS found, but one of its references was not found. In this situation you SHOULD still throw an exception, because Log4JLoggerFactory was found and should not have been. However, your catching Throwable will suppress that exception. IMO, this seems incorrect. Log4JLoggerFactory should just be allowed to propagate.

          I really think ONLY ClassNotFoundException should be caught here. Any other exceptions should be allowed to propagate.

          Show
          Nick Williams added a comment - If NoClassDefFoundError is found here, it should not be caught. It should be allowed to propagate. ClassNotFoundException: Thrown when code attempts to load a class via reflection. This is a checked exception, because the code loading the class could not be checked by the compiler, so it must be checked at runtime. NoClassDefFoundError: This is thrown when code that WAS checked by the compiler fails to load at runtime. This happens when a class that was successfully linked at compile time cannot be located on the class path. For example: public class Animal { public Fur fur; } If Fur cannot be loaded here, a NoClassDefFoundError is thrown. Fur, in this case, was linked at compile time, but that linkage broke at runtime. If Class.forName() throws a NoClassDefFoundError in this case, it could be while loading that class, OR it could be some other major problem with the JRE class path. Importantly, IF NoClassDefFoundError is thrown while loading Log4JLoggerFactory, that means Log4JLoggerFactory WAS found, but one of its references was not found. In this situation you SHOULD still throw an exception, because Log4JLoggerFactory was found and should not have been. However, your catching Throwable will suppress that exception. IMO, this seems incorrect. Log4JLoggerFactory should just be allowed to propagate. I really think ONLY ClassNotFoundException should be caught here. Any other exceptions should be allowed to propagate.
          Hide
          Nick Williams added a comment -

          Correction:

          • Log4JLoggerFactory should just be allowed to propagate.
            + NoClassDefFoundError should just be allowed to propagate.
          Show
          Nick Williams added a comment - Correction: Log4JLoggerFactory should just be allowed to propagate. + NoClassDefFoundError should just be allowed to propagate.
          Hide
          Remko Popma added a comment - - edited

          Thanks for checking. I've followed your advice & the logic now only catches ClassNotFoundEx.
          (Committed revision 1478367)

          Show
          Remko Popma added a comment - - edited Thanks for checking. I've followed your advice & the logic now only catches ClassNotFoundEx. (Committed revision 1478367)

            People

            • Assignee:
              Unassigned
              Reporter:
              Remko Popma
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development