diff --git a/pom.xml b/pom.xml index 1506572..a1d4457 100644 --- a/pom.xml +++ b/pom.xml @@ -283,6 +283,7 @@ under the License. STDOUT --> ${org.apache.logging.log4j:log4j-1.2-api:jar} + ${org.apache.logging.log4j:log4j-api:jar} ${logkit:logkit:jar} ${javax.servlet:javax.servlet-api:jar} target/${project.build.finalName}.jar diff --git a/src/changes/changes.xml b/src/changes/changes.xml index c240e9c..83ef9ac 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,10 +45,11 @@ The type attribute can be add,update,fix,remove. + Fix factory loading from context class loader. Bump org.apache.commons:commons-parent from 71 to 72. - Bump org.slf4j:slf4j-api from 2.0.13 to 2.0.15 #276. + Bump org.slf4j:slf4j-api from 2.0.13 to 2.0.15 #276. diff --git a/src/main/java/org/apache/commons/logging/LogFactory.java b/src/main/java/org/apache/commons/logging/LogFactory.java index bcf0983..238ca9e 100644 --- a/src/main/java/org/apache/commons/logging/LogFactory.java +++ b/src/main/java/org/apache/commons/logging/LogFactory.java @@ -331,8 +331,8 @@ public abstract class LogFactory { + " does not extend '" + LogFactory.class.getName() + "' as loaded by this class loader."); logHierarchy("[BAD CL TREE] ", classLoader); } - - return logFactoryClass.getConstructor().newInstance(); + // Force a ClassCastException + return LogFactory.class.cast(logFactoryClass.getConstructor().newInstance()); } catch (final ClassNotFoundException ex) { if (classLoader == thisClassLoaderRef.get()) { @@ -425,7 +425,8 @@ public abstract class LogFactory { "Unable to load factory class via class loader " + objectId(classLoader) + " - trying the class loader associated with this LogFactory."); } logFactoryClass = Class.forName(factoryClassName); - return logFactoryClass.newInstance(); + // Force a ClassCastException + return LogFactory.class.cast(logFactoryClass.getConstructor().newInstance()); } catch (final Exception e) { // Check to see if we've got a bad configuration if (isDiagnosticsEnabled()) { @@ -796,22 +797,31 @@ public abstract class LogFactory { // Determine whether we will be using the thread context class loader to // load logging classes or not by checking the loaded properties file (if any). - ClassLoader baseClassLoader = contextClassLoader; + boolean useTccl = contextClassLoader != null; if (props != null) { final String useTCCLStr = props.getProperty(TCCL_KEY); - // The Boolean.valueOf(useTCCLStr).booleanValue() formulation - // is required for Java 1.2 compatibility. - if (useTCCLStr != null && !Boolean.parseBoolean(useTCCLStr)) { - // Don't use current context class loader when locating any - // LogFactory or Log classes, just use the class that loaded - // this abstract class. When this class is deployed in a shared - // classpath of a container, it means webapps cannot deploy their - // own logging implementations. It also means that it is up to the - // implementation whether to load library-specific config files - // from the TCCL or not. - baseClassLoader = thisClassLoaderRef.get(); + useTccl &= useTCCLStr == null || Boolean.parseBoolean(useTCCLStr); + } + // If TCCL is still enabled at this point, we check if it resolves this class + if (useTccl) { + try { + if (!LogFactory.class.equals( + Class.forName(LogFactory.class.getName(), false, contextClassLoader))) { + logDiagnostic("The class " + LogFactory.class.getName() + " loaded by the context class loader " + objectId(contextClassLoader) + + " and this class differ. Disabling the usage of the context class loader." + + "Background can be found in https://commons.apache.org/logging/tech.html. "); + logHierarchy("[BAD CL TREE] ", contextClassLoader); + useTccl = false; + } + } catch (ClassNotFoundException ignored) { + logDiagnostic("The class " + LogFactory.class.getName() + " is not present in the the context class loader " + objectId(contextClassLoader) + + ". Disabling the usage of the context class loader." + + "Background can be found in https://commons.apache.org/logging/tech.html. "); + logHierarchy("[BAD CL TREE] ", contextClassLoader); + useTccl = false; } } + final ClassLoader baseClassLoader = useTccl ? contextClassLoader : thisClassLoaderRef.get(); // Determine which concrete LogFactory subclass to use. // First, try a global system property @@ -864,7 +874,7 @@ public abstract class LogFactory { logDiagnostic("[LOOKUP] Using ServiceLoader to define the LogFactory subclass to use..."); } try { - final ServiceLoader serviceLoader = ServiceLoader.load(LogFactory.class); + final ServiceLoader serviceLoader = ServiceLoader.load(LogFactory.class, baseClassLoader); final Iterator iterator = serviceLoader.iterator(); int i = MAX_BROKEN_SERVICES; @@ -923,31 +933,19 @@ public abstract class LogFactory { } } - // Fourth, try one of the 3 provided factories - - try { - // We prefer Log4j API, since it does not stringify objects. - if (factory == null && isClassAvailable(LOG4J_API_LOGGER, baseClassLoader)) { - // If the Log4j API is redirected to SLF4J, we use SLF4J directly. - if (isClassAvailable(LOG4J_TO_SLF4J_BRIDGE, baseClassLoader)) { - logDiagnostic( - "[LOOKUP] Log4j API to SLF4J redirection detected. Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); - factory = newFactory(FACTORY_SLF4J, baseClassLoader, contextClassLoader); - } else { - logDiagnostic("[LOOKUP] Log4j API detected. Loading the Log4j API LogFactory implementation '" + FACTORY_LOG4J_API + "'."); - factory = newFactory(FACTORY_LOG4J_API, baseClassLoader, contextClassLoader); - } - } - - if (factory == null && isClassAvailable(SLF4J_API_LOGGER, baseClassLoader)) { - logDiagnostic("[LOOKUP] SLF4J detected. Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); - factory = newFactory(FACTORY_SLF4J, baseClassLoader, contextClassLoader); - } - } catch (final Exception e) { - logDiagnostic("[LOOKUP] An exception occurred while creating LogFactory: " + e.getMessage()); - } - + // Fourth, try one of the three provided factories first from the specified classloader + // and then from the current one. if (factory == null) { + factory = newStandardFactory(baseClassLoader); + } + if (factory == null) { + factory = newStandardFactory(thisClassLoaderRef.get()); + } + if (factory != null) { + if (isDiagnosticsEnabled()) { + logDiagnostic("Created object " + objectId(factory) + " to manage class loader " + objectId(contextClassLoader)); + } + } else { if (isDiagnosticsEnabled()) { logDiagnostic( "[LOOKUP] Loading the default LogFactory implementation '" + FACTORY_DEFAULT + @@ -1408,6 +1406,45 @@ public abstract class LogFactory { return newFactory(factoryClass, classLoader, null); } + /** + * Tries to load one of the standard three implementations from the given classloader. + *

+ * We assume that {@code classLoader} can load this class. + *

+ * @param classLoader The classloader to use. + * @return An implementation of this class. + */ + private static LogFactory newStandardFactory(final ClassLoader classLoader) { + if (isClassAvailable(LOG4J_TO_SLF4J_BRIDGE, classLoader)) { + try { + return (LogFactory) Class.forName(FACTORY_SLF4J, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic( + "[LOOKUP] Log4j API to SLF4J redirection detected. Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); + } + } + try { + return (LogFactory) Class.forName(FACTORY_LOG4J_API, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic("[LOOKUP] Loading the Log4j API LogFactory implementation '" + FACTORY_LOG4J_API + "'."); + } + try { + return (LogFactory) Class.forName(FACTORY_SLF4J, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic("[LOOKUP] Loading the SLF4J LogFactory implementation '" + FACTORY_SLF4J + "'."); + } + try { + return (LogFactory) Class.forName(FACTORY_DEFAULT, true, classLoader).getConstructor().newInstance(); + } catch (final LinkageError | ReflectiveOperationException ignored) { + } finally { + logDiagnostic("[LOOKUP] Loading the legacy LogFactory implementation '" + FACTORY_DEFAULT + "'."); + } + return null; + } + /** * Gets a new instance of the specified {@code LogFactory} implementation class, loaded by the specified class loader. If that fails, try the class loader * used to load this (abstract) LogFactory. diff --git a/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java b/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java index 5e87db4..aa4005c 100644 --- a/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java +++ b/src/test/java/org/apache/commons/logging/security/SecurityForbiddenTestCase.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertNotEquals; import java.io.PrintWriter; import java.io.StringWriter; import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Hashtable; @@ -93,10 +94,11 @@ public class SecurityForbiddenTestCase extends TestCase { final Class clazz = classLoader.loadClass(name); return clazz.getConstructor().newInstance(); } catch (final Exception e) { + final Throwable wrapped = e instanceof InvocationTargetException ? ((InvocationTargetException) e).getTargetException() : e; final StringWriter sw = new StringWriter(); final PrintWriter pw = new PrintWriter(sw); - e.printStackTrace(pw); - fail("Unexpected exception:" + e.getMessage() + ":" + sw.toString()); + wrapped.printStackTrace(pw); + fail("Unexpected exception:" + wrapped.getMessage() + ":" + sw); } return null; } diff --git a/src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java b/src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java new file mode 100644 index 0000000..2d97056 --- /dev/null +++ b/src/test/java/org/apache/commons/logging/tccl/logfactory/AdaptersTcclTestCase.java @@ -0,0 +1,76 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.logging.tccl.logfactory; + +import junit.framework.Test; +import junit.framework.TestCase; +import org.apache.commons.logging.LogFactory; +import org.apache.commons.logging.PathableClassLoader; +import org.apache.commons.logging.PathableTestSuite; +import org.apache.commons.logging.impl.Log4jApiLogFactory; + +/** + * Verifies that if the TCCL contains only `commons-logging-adapters`, it can load a web-application specific + * logging backend. + */ +public class AdaptersTcclTestCase extends TestCase { + + /** + * Return the tests included in this test suite. + */ + public static Test suite() throws Exception { + // The classloader running the test has access to `commons-logging` + final PathableClassLoader classLoader = new PathableClassLoader(null); + classLoader.useExplicitLoader("junit.", Test.class.getClassLoader()); + classLoader.addLogicalLib("commons-logging"); + classLoader.addLogicalLib("testclasses"); + + // The TCCL has access to both `commons-logging-adapters` and `log4j-api` + final PathableClassLoader tcclLoader = new PathableClassLoader(classLoader); + tcclLoader.addLogicalLib("commons-logging-adapters"); + tcclLoader.addLogicalLib("log4j-api"); + tcclLoader.setParentFirst(false); + + final Class testClass = classLoader.loadClass(AdaptersTcclTestCase.class.getName()); + return new PathableTestSuite(testClass, tcclLoader); + } + + /** + * Sets up instance variables required by this test case. + */ + @Override + public void setUp() throws Exception { + LogFactory.releaseAll(); + } + + /** + * Tear down instance variables required by this test case. + */ + @Override + public void tearDown() { + LogFactory.releaseAll(); + } + + public void testFactoryLoading() { + final LogFactory factory = LogFactory.getFactory(); + // The implementation comes from the TCCL + assertEquals(Thread.currentThread().getContextClassLoader(), factory.getClass().getClassLoader()); + // It uses the `log4j-api` only available in the TCCL + assertEquals(Log4jApiLogFactory.class.getName(), factory.getClass().getName()); + } +} diff --git a/src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java b/src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java new file mode 100644 index 0000000..54e3279 --- /dev/null +++ b/src/test/java/org/apache/commons/logging/tccl/logfactory/SiblingTcclTestCase.java @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.logging.tccl.logfactory; + +import junit.framework.Test; +import junit.framework.TestCase; +import org.apache.commons.logging.LogFactory; +import org.apache.commons.logging.PathableClassLoader; +import org.apache.commons.logging.PathableTestSuite; + +/** + * Verifies that if the TCCL is a sibling of the classloader with `commons-logging` + */ +public class SiblingTcclTestCase extends TestCase { + + /** + * Return the tests included in this test suite. + */ + public static Test suite() throws Exception { + // The classloader running the test has access to `commons-logging` + final PathableClassLoader classLoader = new PathableClassLoader(null); + classLoader.useExplicitLoader("junit.", Test.class.getClassLoader()); + classLoader.addLogicalLib("commons-logging"); + classLoader.addLogicalLib("testclasses"); + + // The TCCL has only access to `log4j-api` and `slf4j-api` + // See https://issues.apache.org/jira/browse/LOGGING-192 + final PathableClassLoader tcclLoader = new PathableClassLoader(null); + tcclLoader.addLogicalLib("log4j-api"); + + final Class testClass = classLoader.loadClass(SiblingTcclTestCase.class.getName()); + return new PathableTestSuite(testClass, tcclLoader); + } + + /** + * Sets up instance variables required by this test case. + */ + @Override + public void setUp() throws Exception { + LogFactory.releaseAll(); + } + + /** + * Tear down instance variables required by this test case. + */ + @Override + public void tearDown() { + LogFactory.releaseAll(); + } + + public void testFactoryLoading() { + // Loading the factory does not fail as in LOGGING-192 + final LogFactory factory = LogFactory.getFactory(); + // The selected implementation comes from this classloader + assertEquals(getClass().getClassLoader(), factory.getClass().getClassLoader()); + } +}