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 16:39:56 GMT
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/284067cb
>> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/284067cb
>>
>> 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>

Mime
View raw message