Good to know. Can the Javadocs be improved? Gary
On Thu, Sep 8, 2016 at 9:39 AM, Matt Sicker <[email protected]> wrote: > 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/2 >>> 84067cb >>> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/2 >>> 84067cb >>> >>> 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]> > -- 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
