Should they have the same name? Gary
On Thu, Sep 8, 2016 at 11:24 AM, Matt Sicker <[email protected]> wrote: > I actually don't know any good reason why they have different names. Most > of the code used to exist in log4j-core, but a bunch of it was useful in > log4j-api, so it's been slowly moved to the util package there. > > On 8 September 2016 at 13:07, Gary Gregory <[email protected]> wrote: > >> I think org.apache.logging.log4j.util.LoaderUtil >> and org.apache.logging.log4j.core.util.Loader. >> >> It's also not clear why one is called "Util" and the other not. >> >> This is all probably nit-picking since these are both internal classes >> but it will help anyone looking at the code including us. >> >> It's always nice to document intent. >> >> Gary >> >> On Thu, Sep 8, 2016 at 10:30 AM, Matt Sicker <[email protected]> wrote: >> >>> 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/async/AsyncLoggerClassLoadDeadlock.java >>>>>>> b/log4j-core/src/test/java/org/apache/logging/log4j/core/asy >>>>>>> nc/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/async/AsyncLoggerClassLoadDeadlockTest.java >>>>>>> b/log4j-core/src/test/java/org/apache/logging/log4j/core/asy >>>>>>> nc/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(Configurati >>>>>>> onFactory.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]> >>> >> >> >> >> -- >> 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
