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

@@ -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"/>

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";

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);
+        }
+    }
+}
+

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();
+        }
+    }
+}