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 17:30:11 GMT
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/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
>



-- 
Matt Sicker <boards@gmail.com>

Mime
View raw message