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/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/284067cb/log4j-core/src/main/java/org/apache/logging/log4j/core/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/impl/ThrowableProxy.java
index a052e4d..e12e14a 100644
--- 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/impl/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/284067cb/log4j-core/src/test/java/org/apache/logging/log4j/core/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/async/AsyncLoggerClassLoadDeadlock.java
new file mode 100644
index 0000000..f8341f9
--- /dev/null
+++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/async/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/284067cb/log4j-core/src/test/java/org/apache/logging/log4j/core/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/async/AsyncLoggerClassLoadDeadlockTest.java
new file mode 100644
index 0000000..990312d
--- /dev/null
+++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/async/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/284067cb/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/284067cb/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>




--



--
Matt Sicker <boards@gmail.com>



--



--
Matt Sicker <boards@gmail.com>