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/ > 284067cb/log4j-core/src/main/java/org/apache/logging/log4j/ > core/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/ > impl/ThrowableProxy.java > index a052e4d..e12e14a 100644 > --- 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/ > impl/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/ > 284067cb/log4j-core/src/test/java/org/apache/logging/log4j/core/async/ > AsyncLoggerClassLoadDeadlock.java > ---------------------------------------------------------------------- > diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/async/ > 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/async/ > 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/ > 284067cb/log4j-core/src/test/java/org/apache/logging/log4j/core/async/ > AsyncLoggerClassLoadDeadlockTest.java > ---------------------------------------------------------------------- > diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/async/ > 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/async/ > 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/ > 284067cb/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/ > 284067cb/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
