logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Sicker <boa...@gmail.com>
Subject Re: logging-log4j2 git commit: Fix class loader deadlock when using async logging and extended stack trace pattern
Date Thu, 08 Sep 2016 18:24:23 GMT
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 <garydgregory@gmail.com> 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 <boards@gmail.com> wrote:
>
>> Which ones?
>>
>> On 8 September 2016 at 12:27, Gary Gregory <garydgregory@gmail.com>
>> wrote:
>>
>>> Good to know. Can the Javadocs be improved?
>>>
>>> Gary
>>>
>>> On Thu, Sep 8, 2016 at 9:39 AM, Matt Sicker <boards@gmail.com> 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 <garydgregory@gmail.com>
>>>> 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, <mattsicker@apache.org> 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 <boards@gmail.com>
>>>>>> Authored: Thu Sep 8 10:30:12 2016 -0500
>>>>>> Committer: Matt Sicker <boards@gmail.com>
>>>>>> 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(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: garydgregory@gmail.com | ggregory@apache.org
>>>>> 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 <boards@gmail.com>
>>>>
>>>
>>>
>>>
>>> --
>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>>> 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 <boards@gmail.com>
>>
>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> 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 <boards@gmail.com>

Mime
View raw message