Which ones? On 8 September 2016 at 12:27, Gary Gregory <[email protected]> wrote:
> 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 > -- Matt Sicker <[email protected]>
