logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@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 17:27:06 GMT
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/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: 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

Mime
View raw message