I'm not using the loader utils here because we already have the ClassLoader required. The use of LoaderUtil tends to be where we don't know which ClassLoader to use.
On 8 September 2016 at 11:09, Gary Gregory <[email protected]> wrote: > Hi Matt, > > When I look at the change in ThrowableProxy that go away from using our > Loader utils and go back to loading classes directly I find it confusing: > Are the Loader utils broken or are these call sites the wrong use case for > the utils? We (you IIRC) have made a point of using these Loader utils (API > and Core) to load classes all over, which is nice since we get consistent > pattern. So I find it confusing to see it done both ways in the code > overall. Does this warrant a comment in the code? It might not, I'm just > asking the question. > > Thank you, > Gary > > > On Thu, Sep 8, 2016 at 8:29 AM, <[email protected]> wrote: > >> Repository: logging-log4j2 >> Updated Branches: >> refs/heads/master 99c0d011d -> 284067cbd >> >> >> Fix class loader deadlock when using async logging and extended stack >> trace pattern >> >> This fixes LOG4J2-1457. >> >> >> Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo >> Commit: http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit >> /284067cb >> Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/284067cb >> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/284067cb >> >> Branch: refs/heads/master >> Commit: 284067cbdb5f13c41225e22c2a60d79cd43f1383 >> Parents: 99c0d01 >> Author: Matt Sicker <[email protected]> >> Authored: Thu Sep 8 10:30:12 2016 -0500 >> Committer: Matt Sicker <[email protected]> >> Committed: Thu Sep 8 10:30:12 2016 -0500 >> >> ---------------------------------------------------------------------- >> .../logging/log4j/core/impl/ThrowableProxy.java | 9 ++-- >> .../async/AsyncLoggerClassLoadDeadlock.java | 32 ++++++++++++++ >> .../async/AsyncLoggerClassLoadDeadlockTest.java | 45 >> ++++++++++++++++++++ >> .../test/resources/AsyncLoggerConsoleTest.xml | 16 +++++++ >> src/changes/changes.xml | 3 ++ >> 5 files changed, 100 insertions(+), 5 deletions(-) >> ---------------------------------------------------------------------- >> >> >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/2 >> 84067cb/log4j-core/src/main/java/org/apache/logging/log4j/co >> re/impl/ThrowableProxy.java >> ---------------------------------------------------------------------- >> diff --git >> a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThrowableProxy.java >> b/log4j-core/src/main/java/org/apache/logging/log4j/core/imp >> l/ThrowableProxy.java >> index a052e4d..e12e14a 100644 >> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/imp >> l/ThrowableProxy.java >> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/imp >> l/ThrowableProxy.java >> @@ -30,7 +30,6 @@ import java.util.Stack; >> >> import org.apache.logging.log4j.core.pattern.PlainTextRenderer; >> import org.apache.logging.log4j.core.pattern.TextRenderer; >> -import org.apache.logging.log4j.core.util.Loader; >> import org.apache.logging.log4j.status.StatusLogger; >> import org.apache.logging.log4j.util.LoaderUtil; >> import org.apache.logging.log4j.util.ReflectionUtil; >> @@ -537,7 +536,7 @@ public class ThrowableProxy implements Serializable { >> Class<?> clazz; >> if (lastLoader != null) { >> try { >> - clazz = Loader.initializeClass(className, lastLoader); >> + clazz = lastLoader.loadClass(className); >> if (clazz != null) { >> return clazz; >> } >> @@ -548,16 +547,16 @@ public class ThrowableProxy implements Serializable >> { >> try { >> clazz = LoaderUtil.loadClass(className); >> } catch (final ClassNotFoundException | NoClassDefFoundError e) { >> - return initializeClass(className); >> + return loadClass(className); >> } catch (final SecurityException e) { >> return null; >> } >> return clazz; >> } >> >> - private Class<?> initializeClass(final String className) { >> + private Class<?> loadClass(final String className) { >> try { >> - return Loader.initializeClass(className, >> this.getClass().getClassLoader()); >> + return this.getClass().getClassLoader >> ().loadClass(className); >> } catch (final ClassNotFoundException | NoClassDefFoundError | >> SecurityException e) { >> return null; >> } >> >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/2 >> 84067cb/log4j-core/src/test/java/org/apache/logging/log4j/co >> re/async/AsyncLoggerClassLoadDeadlock.java >> ---------------------------------------------------------------------- >> diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/asy >> nc/AsyncLoggerClassLoadDeadlock.java b/log4j-core/src/test/java/org >> /apache/logging/log4j/core/async/AsyncLoggerClassLoadDeadlock.java >> new file mode 100644 >> index 0000000..f8341f9 >> --- /dev/null >> +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/asy >> nc/AsyncLoggerClassLoadDeadlock.java >> @@ -0,0 +1,32 @@ >> +/* >> + * 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.logging.log4j.core.async; >> + >> +import org.apache.logging.log4j.LogManager; >> +import org.apache.logging.log4j.Logger; >> + >> +class AsyncLoggerClassLoadDeadlock { >> + static { >> + final Logger log = LogManager.getLogger("com.foo.bar.deadlock"); >> + final Exception e = new Exception(); >> + // the key to reproducing the problem is to fill up the ring >> buffer so that >> + // log.info call will block on ring buffer as well >> + for (int i = 0; i < >> AsyncLoggerClassLoadDeadlockTest.RING_BUFFER_SIZE >> * 2; ++i) { >> + log.info("clinit", e); >> + } >> + } >> +} >> >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/2 >> 84067cb/log4j-core/src/test/java/org/apache/logging/log4j/co >> re/async/AsyncLoggerClassLoadDeadlockTest.java >> ---------------------------------------------------------------------- >> diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/asy >> nc/AsyncLoggerClassLoadDeadlockTest.java b/log4j-core/src/test/java/org >> /apache/logging/log4j/core/async/AsyncLoggerClassLoadDeadlockTest.java >> new file mode 100644 >> index 0000000..990312d >> --- /dev/null >> +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/asy >> nc/AsyncLoggerClassLoadDeadlockTest.java >> @@ -0,0 +1,45 @@ >> +/* >> + * 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.logging.log4j.core.async; >> + >> +import org.apache.logging.log4j.core.config.ConfigurationFactory; >> +import org.junit.BeforeClass; >> +import org.junit.Test; >> + >> +import static org.junit.Assert.assertNotNull; >> + >> +/** >> + * Test class loading deadlock condition from the LOG4J2-1457 >> + */ >> +public class AsyncLoggerClassLoadDeadlockTest { >> + >> + static final int RING_BUFFER_SIZE = 128; >> + >> + @BeforeClass >> + public static void beforeClass() { >> + System.setProperty("Log4jContextSelector", >> "org.apache.logging.log4j.core.async.AsyncLoggerContextSelector"); >> + System.setProperty("AsyncLogger.RingBufferSize", >> String.valueOf(RING_BUFFER_SIZE)); >> + System.setProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY, >> "AsyncLoggerConsoleTest.xml"); >> + } >> + >> + @Test(timeout = 30000) >> + public void testClassLoaderDeadlock() throws Exception { >> + //touch the class so static init will be called >> + final AsyncLoggerClassLoadDeadlock temp = new >> AsyncLoggerClassLoadDeadlock(); >> + assertNotNull(temp); >> + } >> +} >> >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/2 >> 84067cb/log4j-core/src/test/resources/AsyncLoggerConsoleTest.xml >> ---------------------------------------------------------------------- >> diff --git a/log4j-core/src/test/resources/AsyncLoggerConsoleTest.xml >> b/log4j-core/src/test/resources/AsyncLoggerConsoleTest.xml >> new file mode 100644 >> index 0000000..90f4e9e >> --- /dev/null >> +++ b/log4j-core/src/test/resources/AsyncLoggerConsoleTest.xml >> @@ -0,0 +1,16 @@ >> +<?xml version="1.0" encoding="UTF-8"?> >> +<Configuration status="warn"> >> + <Appenders> >> + <Console name="Console" target="SYSTEM_OUT"> >> + <PatternLayout> >> + <pattern>%xEx{0}</pattern> >> + </PatternLayout> >> + </Console> >> + </Appenders> >> + >> + <Loggers> >> + <Root level="info"> >> + <AppenderRef ref="Console"/> >> + </Root> >> + </Loggers> >> +</Configuration> >> >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/2 >> 84067cb/src/changes/changes.xml >> ---------------------------------------------------------------------- >> diff --git a/src/changes/changes.xml b/src/changes/changes.xml >> index 2bfc311..fa53129 100644 >> --- a/src/changes/changes.xml >> +++ b/src/changes/changes.xml >> @@ -24,6 +24,9 @@ >> </properties> >> <body> >> <release version="2.7" date="2016-MM-DD" description="GA Release >> 2.7"> >> + <action issue="LOG4J2-1457" dev="mattsicker" type="fix" >> due-to="Leon Finker"> >> + Class loader deadlock when using async logging and extended >> stack trace pattern. >> + </action> >> <action issue="LOG4J2-1563" dev="ggregory" type="fix" >> due-to="Jason Tedor"> >> Log4j 2.6.2 can lose exceptions when a security manager is >> present. >> </action> >> >> > > > -- > E-Mail: [email protected] | [email protected] > Java Persistence with Hibernate, Second Edition > <http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory > -- Matt Sicker <[email protected]>
