Return-Path: 
 <commons-dev-return-62525-apmail-jakarta-commons-dev-archive=jakarta.apache.org@jakarta.apache.org>
Delivered-To: apmail-jakarta-commons-dev-archive@www.apache.org
Received: (qmail 75680 invoked from network); 17 Nov 2004 23:23:28 -0000
Received: from hermes.apache.org (HELO mail.apache.org) (209.237.227.199)
  by minotaur-2.apache.org with SMTP; 17 Nov 2004 23:23:28 -0000
Received: (qmail 5500 invoked by uid 500); 17 Nov 2004 23:23:25 -0000
Delivered-To: apmail-jakarta-commons-dev-archive@jakarta.apache.org
Received: (qmail 5310 invoked by uid 500); 17 Nov 2004 23:23:24 -0000
Mailing-List: contact commons-dev-help@jakarta.apache.org; run by ezmlm
Precedence: bulk
List-Unsubscribe: <mailto:commons-dev-unsubscribe@jakarta.apache.org>
List-Subscribe: <mailto:commons-dev-subscribe@jakarta.apache.org>
List-Help: <mailto:commons-dev-help@jakarta.apache.org>
List-Post: <mailto:commons-dev@jakarta.apache.org>
List-Id: "Jakarta Commons Developers List" <commons-dev.jakarta.apache.org>
Reply-To: "Jakarta Commons Developers List" <commons-dev@jakarta.apache.org>
Delivered-To: mailing list commons-dev@jakarta.apache.org
Received: (qmail 5297 invoked by uid 500); 17 Nov 2004 23:23:24 -0000
Received: (qmail 5294 invoked by uid 99); 17 Nov 2004 23:23:24 -0000
X-ASF-Spam-Status: No, hits=-10.0 required=10.0
	tests=ALL_TRUSTED,NO_REAL_NAME
X-Spam-Check-By: apache.org
Received: from [209.237.227.194] (HELO minotaur.apache.org) (209.237.227.194)
  by apache.org (qpsmtpd/0.28) with SMTP; Wed, 17 Nov 2004 15:23:23 -0800
Received: (qmail 75616 invoked by uid 1289); 17 Nov 2004 23:23:22 -0000
Date: 17 Nov 2004 23:23:22 -0000
Message-ID: <20041117232322.75615.qmail@minotaur.apache.org>
From: rdonkin@apache.org
To: jakarta-commons-cvs@apache.org
Subject: cvs commit:
 jakarta-commons/logging/optional/src/test/org/apache/commons/logging/impl
 WeakHashtableTest.java
X-Virus-Checked: Checked
X-Spam-Rating: minotaur-2.apache.org 1.6.2 0/1000/N

rdonkin     2004/11/17 15:23:22

  Modified:    logging/optional/src/java/org/apache/commons/logging/impl
                        WeakHashtable.java
               logging/optional/src/test/org/apache/commons/logging
                        LogFactoryTest.java
               logging/optional/src/test/org/apache/commons/logging/impl
                        WeakHashtableTest.java
  Log:
  Improvements to WeakHashTable. Values are now held with hard references and a reference queue is polled during a purge. Contributed by Brian Stansberry.
  
  Revision  Changes    Path
  1.4       +120 -27   jakarta-commons/logging/optional/src/java/org/apache/commons/logging/impl/WeakHashtable.java
  
  Index: WeakHashtable.java
  ===================================================================
  RCS file: /home/cvs/jakarta-commons/logging/optional/src/java/org/apache/commons/logging/impl/WeakHashtable.java,v
  retrieving revision 1.3
  retrieving revision 1.4
  diff -u -r1.3 -r1.4
  --- WeakHashtable.java	11 Nov 2004 22:31:05 -0000	1.3
  +++ WeakHashtable.java	17 Nov 2004 23:23:21 -0000	1.4
  @@ -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 @@
    * 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 @@
        *@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");
  @@ -177,9 +193,17 @@
           if (value == null) {
               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 @@
       }
       
       /**
  -     * 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();
  -        for (Iterator it=entrySet.iterator(); it.hasNext();) {
  -            Map.Entry entry = (Map.Entry) it.next();
  -            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();
  -            }
  +        WeakKey key;
  +        while ( (key = (WeakKey) queue.poll()) != null) {
  +            super.remove(key.getReferenced());
           }
       }
       
  -    
  -    
       /** Entry implementation */
       private final static class Entry implements Map.Entry {
       
  -        private Object key;
  -        private Object value;
  +        private final Object key;
  +        private final Object value;
           
           private Entry(Object key, Object value) {
               this.key = key;
  @@ -318,18 +341,33 @@
       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;
  -            Object keyValue = getValue();
  -            if (keyValue != null) {
  -                result = keyValue.hashCode();
  -            }
  -            return result;
  +            return hashCode;
           }
           
           private Object getValue() {
  @@ -342,8 +380,22 @@
                   Referenced otherKey = (Referenced) o;
                   Object thisKeyValue = getValue();
                   Object otherKeyValue = otherKey.getValue();
  -                if (thisKeyValue == null) {
  +                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 @@
               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;
  +        //}      
  +     }
   }
  
  
  
  1.2       +174 -2    jakarta-commons/logging/optional/src/test/org/apache/commons/logging/LogFactoryTest.java
  
  Index: LogFactoryTest.java
  ===================================================================
  RCS file: /home/cvs/jakarta-commons/logging/optional/src/test/org/apache/commons/logging/LogFactoryTest.java,v
  retrieving revision 1.1
  retrieving revision 1.2
  diff -u -r1.1 -r1.2
  --- LogFactoryTest.java	10 Nov 2004 22:59:39 -0000	1.1
  +++ LogFactoryTest.java	17 Nov 2004 23:23:21 -0000	1.2
  @@ -17,8 +17,12 @@
   
   package org.apache.commons.logging;
   
  -import junit.framework.*;
  -import java.util.*;
  +import java.lang.ref.WeakReference;
  +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 @@
       /** 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 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();
  +    }
  +    
   }
  
  
  
  1.4       +11 -1     jakarta-commons/logging/optional/src/test/org/apache/commons/logging/impl/WeakHashtableTest.java
  
  Index: WeakHashtableTest.java
  ===================================================================
  RCS file: /home/cvs/jakarta-commons/logging/optional/src/test/org/apache/commons/logging/impl/WeakHashtableTest.java,v
  retrieving revision 1.3
  retrieving revision 1.4
  diff -u -r1.3 -r1.4
  --- WeakHashtableTest.java	11 Nov 2004 22:31:05 -0000	1.3
  +++ WeakHashtableTest.java	17 Nov 2004 23:23:22 -0000	1.4
  @@ -17,6 +17,7 @@
   
   package org.apache.commons.logging.impl;
   
  +import java.lang.ref.*;
   import junit.framework.*;
   import java.util.*;
   
  @@ -206,7 +207,10 @@
       }
       
       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;
           keyTwo = null;
  @@ -232,6 +236,12 @@
                   bytz = bytz * 2;
               }
           }
  +        
  +        // 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());
  
  
  

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org