Improvements to WeakHashTable. Values are now held with hard references and a reference queue is polled during a purge. Contributed by Brian Stansberry.

git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/logging/trunk@139059 13f79535-47bb-0310-9956-ffa450edef68

optional/src/java/org/apache/commons/logging/impl/WeakHashtable.java

@@ -17,6 +17,7 @@
 
 package org.apache.commons.logging.impl;
 
+import java.lang.ref.ReferenceQueue;
 import java.lang.ref.WeakReference;
 import java.util.*;
 
@@ -37,12 +38,28 @@ import java.util.*;
  * running 1.3+ JVMs. Use of this class will allow classloaders to be collected by 
  * the garbage collector without the need to call release.
  * </p>
+ * 
+ * @author Brian Stansberry
  */
 public final class WeakHashtable extends Hashtable {
 
     /** Empty array of <code>Entry</code>'s */
     private static final Entry[] EMPTY_ENTRY_ARRAY = {};
+    /** 
+     * The maximum number of times put() can be called before
+     * the map will purged of cleared entries.
+     */
+    public static final int MAX_PUTS_BEFORE_PURGE = 100;
 
+    /* ReferenceQueue we check for gc'd keys */
+    private ReferenceQueue queue = new ReferenceQueue();
+    /* Counter used to control how often we purge gc'd entries */
+    private int putCount = 0;
+    
+    /**
+     * Constructs a WeakHashtable with the Hashtable default
+     * capacity and load factor.
+     */
     public WeakHashtable() {}
 
     /**
@@ -169,7 +186,6 @@ public final class WeakHashtable extends Hashtable {
      *@see Hashtable
      */    
     public Object put(Object key, Object value) {
-        // for performance reasons, no purge
         // check for nulls, ensuring symantics match superclass
         if (key == null) {
             throw new NullPointerException("Null keys are not allowed");
@@ -178,8 +194,16 @@ public final class WeakHashtable extends Hashtable {
             throw new NullPointerException("Null values are not allowed");
         }
 
+        // for performance reasons, only purge every 
+        // MAX_PUTS_BEFORE_PURGE times
+        if (putCount++ > MAX_PUTS_BEFORE_PURGE) {
+            purge();
+            putCount = 0;
+        }
         Object result = null;
-        Referenced lastValue = (Referenced) super.put(new Referenced(key), new Referenced(value));
+        Referenced keyRef    = new Referenced(key, value, queue);
+        Referenced valueRef  = new Referenced(value);
+        Referenced lastValue = (Referenced) super.put(keyRef, valueRef);
         if (lastValue != null) {
             result = lastValue.getValue();
         }
@@ -248,31 +272,30 @@ public final class WeakHashtable extends Hashtable {
     }
     
     /**
-     * Purges all entries whose wrapped keys or values
+     * @see Hashtable
+     */
+    protected void rehash() {
+        // purge here to save the effort of rehashing dead entries
+        purge();
+        super.rehash();
+    }
+    
+    /**
+     * Purges all entries whose wrapped keys
      * have been garbage collected.
      */
     private synchronized void purge() {
-        Set entrySet = super.entrySet();
+        WeakKey key;
-        for (Iterator it=entrySet.iterator(); it.hasNext();) {
+        while ( (key = (WeakKey) queue.poll()) != null) {
-            Map.Entry entry = (Map.Entry) it.next();
+            super.remove(key.getReferenced());
-            Referenced referencedKey = (Referenced) entry.getKey();
-            Referenced referencedValue = (Referenced) entry.getValue();
-            
-            // test whether either referant has been collected
-            if (referencedKey.getValue() == null || referencedValue.getValue() == null) {
-                // if so, purge this entry
-                it.remove();
-            }
         }
     }
     
-    
-    
     /** Entry implementation */
     private final static class Entry implements Map.Entry {
     
-        private Object key;
+        private final Object key;
-        private Object value;
+        private final Object value;
         
         private Entry(Object key, Object value) {
             this.key = key;
@@ -318,18 +341,33 @@ public final class WeakHashtable extends Hashtable {
     private final static class Referenced {
         
         private final WeakReference reference;
+        private final int           hashCode;
 
+        /**
+         * 
+         * @throws NullPointerException if referant is <code>null</code>
+         */        
         private Referenced(Object referant) {
             reference = new WeakReference(referant);
+            // Calc a permanent hashCode so calls to Hashtable.remove()
+            // work if the WeakReference has been cleared
+            hashCode  = referant.hashCode();
+        }
+        
+        /**
+         * 
+         * @throws NullPointerException if key is <code>null</code>
+         */
+        private Referenced(Object key, Object value, ReferenceQueue queue) {
+            reference = new WeakKey(key, value, queue, this);
+            // Calc a permanent hashCode so calls to Hashtable.remove()
+            // work if the WeakReference has been cleared
+            hashCode  = key.hashCode();
+
         }
         
         public int hashCode() {
-            int result = 0;
+            return hashCode;
-            Object keyValue = getValue();
-            if (keyValue != null) {
-                result = keyValue.hashCode();
-            }
-            return result;
         }
         
         private Object getValue() {
@@ -344,6 +382,20 @@ public final class WeakHashtable extends Hashtable {
                 Object otherKeyValue = otherKey.getValue();
                 if (thisKeyValue == null) {                     
                     result = (otherKeyValue == null);
+                    
+                    // Since our hashcode was calculated from the original
+                    // non-null referant, the above check breaks the 
+                    // hashcode/equals contract, as two cleared Referenced
+                    // objects could test equal but have different hashcodes.
+                    // We can reduce (not eliminate) the chance of this
+                    // happening by comparing hashcodes.
+                    if (result == true) {
+                        result = (this.hashCode() == otherKey.hashCode());
+                    }
+                    // In any case, as our c'tor does not allow null referants
+                    // and Hashtable does not do equality checks between 
+                    // existing keys, normal hashtable operations should never 
+                    // result in an equals comparison between null referants
                 }
                 else
                 {
@@ -353,4 +405,45 @@ public final class WeakHashtable extends Hashtable {
             return result;
         }
     }
+    
+    /**
+     * WeakReference subclass that holds a hard reference to an
+     * associated <code>value</code> and also makes accessible
+     * the Referenced object holding it.
+     */
+    private final static class WeakKey extends WeakReference {
+        
+        private final Object     hardValue;
+        private final Referenced referenced;
+        
+        private WeakKey(Object key, 
+                        Object value, 
+                        ReferenceQueue queue,
+                        Referenced referenced) {
+            super(key, queue);
+            hardValue = value;
+            this.referenced = referenced;
+        }
+        
+        private Referenced getReferenced() {
+            return referenced;
+        }
+        
+        /* Drop our hard reference to value if we've been cleared
+         * by the gc.
+         * 
+         * Testing shows that with key objects like ClassLoader
+         * that don't override hashCode(), get() is never
+         * called once the key is in a Hashtable. 
+         * So, this method override is commented out. 
+         */
+        //public Object get() {
+        //    Object result = super.get();
+        //    if (result == null) {
+        //        // We've been cleared, so drop our hard reference to value
+        //        hardValue = null;
+        //    }
+        //    return result;
+        //}      
+     }
 }

optional/src/test/org/apache/commons/logging/LogFactoryTest.java

@@ -17,8 +17,12 @@
 
 package org.apache.commons.logging;
 
-import junit.framework.*;
+import java.lang.ref.WeakReference;
-import java.util.*;
+import java.util.Hashtable;
+
+import junit.framework.TestCase;
+
+import org.apache.commons.logging.impl.LogFactoryImpl;
 import org.apache.commons.logging.impl.WeakHashtable;
 
 public class LogFactoryTest extends TestCase {
@@ -27,6 +31,8 @@ public class LogFactoryTest extends TestCase {
     /** Maximum number of iterations before our test fails */
     private static final int MAX_GC_ITERATIONS = 50;
 
+    private ClassLoader origLoader          = null;
+    private String      origFactoryProperty = null;
 
     public LogFactoryTest(String testName) {
         super(testName);
@@ -35,4 +41,170 @@ public class LogFactoryTest extends TestCase {
     public void testLogFactoryType() {
         assertTrue(LogFactory.factories instanceof WeakHashtable);
     }
+    
+    /**
+     * Tests that LogFactories are not removed from the map
+     * if their creating ClassLoader is still alive.
+     */ 
+    public void testHoldFactories()
+    {     
+        // Get a factory and create a WeakReference to it that
+        // we can check to see if the factory has been removed
+        // from LogFactory.properties
+        LogFactory factory = LogFactory.getFactory();
+        WeakReference weakFactory = new WeakReference(factory);
+        
+        // Remove any hard reference to the factory
+        factory = null; 
+        
+        // Run the gc, confirming that the original factory
+        // is not dropped from the map even though there are 
+        // no other references to it
+        int iterations = 0;
+        int bytz = 2;
+        while(iterations++ < MAX_GC_ITERATIONS) {
+            System.gc();
+            
+            assertNotNull("LogFactory released while ClassLoader still active.",
+                          weakFactory.get());                
+            
+            // create garbage:
+            byte[] b;
+            try {
+              b  =  new byte[bytz];            
+              bytz = bytz * 2;
+            }
+            catch (OutOfMemoryError oom) {
+                // This error means the test passed, as it means the LogFactory
+                // was never released.  So, we have to catch and deal with it
+                
+                // Doing this is probably a no-no, but it seems to work ;-)
+                b = null;
+                System.gc();
+                break;
+            }
+        }
+    }
+    
+    /**
+     * Tests that a LogFactory is eventually removed from the map
+     * after its creating ClassLoader is garbage collected. 
+     */
+    public void testReleaseFactories()
+    {
+        // Create a temporary classloader        
+        ClassLoader childLoader = new ClassLoader() {};
+        Thread.currentThread().setContextClassLoader(childLoader);
+        
+        // Get a factory using the child loader.
+        LogFactory factory        = LogFactory.getFactory();
+        // Hold a WeakReference to the factory. When this reference
+        // is cleared we know the factory has been cleared from
+        // LogFactory.factories as well
+        WeakReference weakFactory = new WeakReference(factory);
+        
+        // Get a WeakReference to the child loader so we know when it
+        // has been gc'ed
+        WeakReference weakLoader = new WeakReference(childLoader);
+        
+        // Remove any hard reference to the childLoader and the factory
+        Thread.currentThread().setContextClassLoader(origLoader);
+        childLoader = null;
+        factory     = null;
+        
+        // Run the gc, confirming that the original childLoader
+        // is dropped from the map
+        int iterations = 0;
+        int bytz = 2;
+        while(true) {
+            System.gc();
+            if(iterations++ > MAX_GC_ITERATIONS){
+                fail("Max iterations reached before childLoader released.");
+            }
+            
+            if(weakLoader.get() == null) {
+                break;                
+            } else {
+                // create garbage:
+                byte[] b;
+                try {
+                    b =  new byte[bytz];
+                    bytz = bytz * 2;
+                }
+                catch (OutOfMemoryError oom) {
+                    // Doing this is probably a no-no, but it seems to work ;-)
+                    b = null;
+                    System.gc();
+                    fail("OutOfMemory before childLoader released.");
+                }
+            }
+        }
+        
+        // Confirm that the original factory is removed from the map
+        // within the maximum allowed number of calls to put() +
+        // the maximum number of subsequent gc iterations
+        iterations = 0;
+        while(true) {
+            System.gc();
+            if(iterations++ > WeakHashtable.MAX_PUTS_BEFORE_PURGE + MAX_GC_ITERATIONS){
+                Hashtable table = LogFactory.factories;
+                fail("Max iterations reached before factory released.");
+            }           
+            
+            // Create a new child loader and use it to add to the map.
+            ClassLoader newChildLoader  = new ClassLoader() {};
+            Thread.currentThread().setContextClassLoader(newChildLoader);
+            LogFactory.getFactory();
+            Thread.currentThread().setContextClassLoader(origLoader);
+            
+            if(weakFactory.get() == null) {
+                break;                
+            } else {
+                // create garbage:
+                byte[] b;
+                try {
+                    b =  new byte[bytz];
+                    bytz = bytz * 2;
+                }
+                catch (OutOfMemoryError oom) {
+                    // Doing this is probably a no-no, but it seems to work ;-)
+                    b = null;
+                    bytz = 2; // start over
+                    System.gc();
+                }
+            }
+        }
+        
+    }    
+    
+    protected void setUp() throws Exception {
+        // Preserve the original classloader and factory implementation
+        // class so we can restore them when we are done
+        origLoader          = Thread.currentThread().getContextClassLoader();
+        origFactoryProperty = System.getProperty(LogFactory.FACTORY_PROPERTY);
+        
+        // Ensure we use LogFactoryImpl as our factory
+        System.setProperty(LogFactory.FACTORY_PROPERTY, 
+                           LogFactoryImpl.class.getName());
+        
+        super.setUp();
+    }
+    
+    protected void tearDown() throws Exception {
+        // Set the  classloader back to whatever it originally was
+        Thread.currentThread().setContextClassLoader(origLoader);
+        
+        // Set the factory implementation class back to 
+        // whatever it originally was
+        if (origFactoryProperty != null) {
+            System.setProperty(LogFactory.FACTORY_PROPERTY, 
+                               origFactoryProperty);
+        }
+        else {
+            System.getProperties().remove(LogFactory.FACTORY_PROPERTY);
+        }
+        
+        super.tearDown();
+    }
+    
 }

optional/src/test/org/apache/commons/logging/impl/WeakHashtableTest.java

@@ -17,6 +17,7 @@
 
 package org.apache.commons.logging.impl;
 
+import java.lang.ref.*;
 import junit.framework.*;
 import java.util.*;
 
@@ -206,6 +207,9 @@ public class WeakHashtableTest extends TestCase {
     }
     
     public void testRelease() throws Exception {
+        assertNotNull(weakHashtable.get(new Long(1)));
+        ReferenceQueue testQueue = new ReferenceQueue();
+        WeakReference weakKeyOne = new WeakReference(keyOne, testQueue);
 
         // lose our references
         keyOne = null;
@@ -233,6 +237,12 @@ public class WeakHashtableTest extends TestCase {
             }
         }
         
+        // some JVMs seem to take a little time to put references on 
+        // the reference queue once the reference has been collected
+        // need to think about whether this is enough to justify
+        // stepping through the collection each time...
+        while(testQueue.poll() == null) {}
+        
         // Test that the released objects are not taking space in the table
         assertEquals("underlying table not emptied", 0, weakHashtable.size());
     }