You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2022/01/05 14:35:15 UTC
[commons-logging] branch master updated: Use a weak reference for the cached class loader (#71)
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-logging.git
The following commit(s) were added to refs/heads/master by this push:
new e7b328d Use a weak reference for the cached class loader (#71)
e7b328d is described below
commit e7b328d7e03f5bd9ea1d40169e8dccd3abddda05
Author: Jakob van Kruijssen <ca...@gmail.com>
AuthorDate: Wed Jan 5 15:35:08 2022 +0100
Use a weak reference for the cached class loader (#71)
This replaces the strong reference to the class loader, `thisClassLoader`,
with a weak one.
The strong ref shows up as causing a GC root after unloading a web app in Tomcat that uses this library.
With these modifications, the GC root is gone...
---
build-testing.xml | 2 +-
.../org/apache/commons/logging/LogFactory.java | 24 +++---
.../commons/logging/GarbageCollectionHelper.java | 86 ++++++++++++++++++++++
.../logging/LogFactoryWeakReferenceTestCase.java | 65 ++++++++++++++++
4 files changed, 165 insertions(+), 12 deletions(-)
diff --git a/build-testing.xml b/build-testing.xml
index ecc8bb1..30e5fab 100644
--- a/build-testing.xml
+++ b/build-testing.xml
@@ -100,7 +100,7 @@
<property name="component.title" value="Logging Wrapper Library"/>
<!-- The current version number of this component -->
- <property name="component.version" value="1.1.1-SNAPSHOT"/>
+ <property name="component.version" value="1.2.1-SNAPSHOT"/>
<!-- The base directory for compilation targets -->
<property name="build.home" value="${basedir}/target"/>
diff --git a/src/main/java/org/apache/commons/logging/LogFactory.java b/src/main/java/org/apache/commons/logging/LogFactory.java
index 2ffb23f..d5594e7 100644
--- a/src/main/java/org/apache/commons/logging/LogFactory.java
+++ b/src/main/java/org/apache/commons/logging/LogFactory.java
@@ -23,6 +23,7 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.PrintStream;
+import java.lang.ref.WeakReference;
import java.net.URL;
import java.net.URLConnection;
import java.security.AccessController;
@@ -188,7 +189,7 @@ public abstract class LogFactory {
* AccessControllers etc. It's more efficient to compute it once and
* cache it here.
*/
- private static final ClassLoader thisClassLoader;
+ private static final WeakReference<ClassLoader> thisClassLoaderRef;
// ----------------------------------------------------------- Constructors
@@ -419,6 +420,7 @@ public abstract class LogFactory {
// Identify the class loader we will be using
final ClassLoader contextClassLoader = getContextClassLoaderInternal();
+
// This is an odd enough situation to report about. This
// output will be a nuisance on JDK1.1, as the system
// classloader is null in that environment.
@@ -466,7 +468,7 @@ public abstract class LogFactory {
// own logging implementations. It also means that it is up to the
// implementation whether to load library-specific config files
// from the TCCL or not.
- baseClassLoader = thisClassLoader;
+ baseClassLoader = thisClassLoaderRef.get();
}
}
@@ -622,7 +624,7 @@ public abstract class LogFactory {
// version of the LogFactoryImpl class and have it used dynamically
// by an old LogFactory class in the parent, but that isn't
// necessarily a good idea anyway.
- factory = newFactory(FACTORY_DEFAULT, thisClassLoader, contextClassLoader);
+ factory = newFactory(FACTORY_DEFAULT, thisClassLoaderRef.get(), contextClassLoader);
}
if (factory != null) {
@@ -1049,7 +1051,7 @@ public abstract class LogFactory {
return (LogFactory) logFactoryClass.newInstance();
} catch (final ClassNotFoundException ex) {
- if (classLoader == thisClassLoader) {
+ if (classLoader == thisClassLoaderRef.get()) {
// Nothing more to try, onwards.
if (isDiagnosticsEnabled()) {
logDiagnostic("Unable to locate any class called '" + factoryClass +
@@ -1059,7 +1061,7 @@ public abstract class LogFactory {
}
// ignore exception, continue
} catch (final NoClassDefFoundError e) {
- if (classLoader == thisClassLoader) {
+ if (classLoader == thisClassLoaderRef.get()) {
// Nothing more to try, onwards.
if (isDiagnosticsEnabled()) {
logDiagnostic("Class '" + factoryClass + "' cannot be loaded" +
@@ -1070,9 +1072,9 @@ public abstract class LogFactory {
}
// ignore exception, continue
} catch (final ClassCastException e) {
- if (classLoader == thisClassLoader) {
+ if (classLoader == thisClassLoaderRef.get()) {
// There's no point in falling through to the code below that
- // tries again with thisClassLoader, because we've just tried
+ // tries again with thisClassLoaderRef, because we've just tried
// loading with that loader (not the TCCL). Just throw an
// appropriate exception here.
@@ -1111,7 +1113,7 @@ public abstract class LogFactory {
}
// Ignore exception, continue. Presumably the classloader was the
- // TCCL; the code below will try to load the class via thisClassLoader.
+ // TCCL; the code below will try to load the class via thisClassLoaderRef.
// This will handle the case where the original calling class is in
// a shared classpath but the TCCL has a copy of LogFactory and the
// specified LogFactory implementation; we will fall back to using the
@@ -1674,7 +1676,8 @@ public abstract class LogFactory {
static {
// note: it's safe to call methods before initDiagnostics (though
// diagnostic output gets discarded).
- thisClassLoader = getClassLoader(LogFactory.class);
+ ClassLoader thisClassLoader = getClassLoader(LogFactory.class);
+ thisClassLoaderRef = new WeakReference<ClassLoader>(thisClassLoader);
// In order to avoid confusion where multiple instances of JCL are
// being used via different classloaders within the same app, we
// ensure each logged message has a prefix of form
@@ -1686,11 +1689,10 @@ public abstract class LogFactory {
// output diagnostics from this class are static.
String classLoaderName;
try {
- final ClassLoader classLoader = thisClassLoader;
if (thisClassLoader == null) {
classLoaderName = "BOOTLOADER";
} else {
- classLoaderName = objectId(classLoader);
+ classLoaderName = objectId(thisClassLoader);
}
} catch (final SecurityException e) {
classLoaderName = "UNKNOWN";
diff --git a/src/test/java/org/apache/commons/logging/GarbageCollectionHelper.java b/src/test/java/org/apache/commons/logging/GarbageCollectionHelper.java
new file mode 100644
index 0000000..75d449d
--- /dev/null
+++ b/src/test/java/org/apache/commons/logging/GarbageCollectionHelper.java
@@ -0,0 +1,86 @@
+/*
+ * 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.commons.logging;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+// after: https://github.com/apache/logging-log4j2/blob/c47e98423b461731f7791fcb9ea1079cd451f365/log4j-core/src/test/java/org/apache/logging/log4j/core/GarbageCollectionHelper.java
+public final class GarbageCollectionHelper implements Closeable, Runnable {
+ private static final OutputStream SINK = new OutputStream() {
+ @Override
+ public void write(int b) {
+ }
+
+ @Override
+ public void write(byte[] b) {
+ }
+
+ @Override
+ public void write(byte[] b, int off, int len) {
+ }
+ };
+ private final AtomicBoolean running = new AtomicBoolean();
+ private final CountDownLatch latch = new CountDownLatch(1);
+ private final Thread gcThread = new Thread(new GcTask());
+
+ class GcTask implements Runnable {
+ @Override
+ public void run() {
+ try {
+ while (running.get()) {
+ // Allocate data to help suggest a GC
+ try {
+ // 1mb of heap
+ byte[] buf = new byte[1024 * 1024];
+ SINK.write(buf);
+ } catch (final IOException ignored) {
+ }
+ // May no-op depending on the JVM configuration
+ System.gc();
+ }
+ } finally {
+ latch.countDown();
+ }
+ }
+ }
+
+ @Override
+ public void run() {
+ if (running.compareAndSet(false, true)) {
+ gcThread.start();
+ }
+ }
+
+ @Override
+ public void close() {
+ running.set(false);
+ try {
+ junit.framework.TestCase.assertTrue("GarbageCollectionHelper did not shut down cleanly",
+ latch.await(10, TimeUnit.SECONDS));
+ } catch (final InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }
+}
+
diff --git a/src/test/java/org/apache/commons/logging/LogFactoryWeakReferenceTestCase.java b/src/test/java/org/apache/commons/logging/LogFactoryWeakReferenceTestCase.java
new file mode 100644
index 0000000..43c475c
--- /dev/null
+++ b/src/test/java/org/apache/commons/logging/LogFactoryWeakReferenceTestCase.java
@@ -0,0 +1,65 @@
+/*
+ * 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.commons.logging;
+
+import java.lang.ref.WeakReference;
+import java.lang.reflect.Field;
+
+import junit.framework.TestCase;
+
+public class LogFactoryWeakReferenceTestCase extends TestCase {
+ private static final long MAX_WAIT_FOR_REF_NULLED_BY_GC = 15000;
+
+ public void testNotLeakingThisClassLoader() throws Exception {
+ // create an isolated loader
+ PathableClassLoader loader = new PathableClassLoader(null);
+ loader.addLogicalLib("commons-logging");
+
+ // load the LogFactory class through this loader
+ Class<?> logFactoryClass = loader.loadClass(LogFactory.class.getName());
+
+ // reflection hacks to obtain the weak reference
+ Field field = logFactoryClass.getDeclaredField("thisClassLoaderRef");
+ field.setAccessible(true);
+ WeakReference thisClassLoaderRef = (WeakReference) field.get(null);
+
+ // the ref should at this point contain the loader
+ assertSame(loader, thisClassLoaderRef.get());
+
+ // null out the hard refs
+ field = null;
+ logFactoryClass = null;
+ loader.close();
+ loader = null;
+
+ GarbageCollectionHelper gcHelper = new GarbageCollectionHelper();
+ gcHelper.run();
+ try {
+ long start = System.currentTimeMillis();
+ while (thisClassLoaderRef.get() != null) {
+ if (System.currentTimeMillis() - start > MAX_WAIT_FOR_REF_NULLED_BY_GC) {
+ fail("After waiting " + MAX_WAIT_FOR_REF_NULLED_BY_GC + "ms, the weak ref still yields a non-null value.");
+ }
+ Thread.sleep(100);
+ }
+ } finally {
+ gcHelper.close();
+ }
+ }
+}