210470 merged into refactoring branch

5 files changed, 167 insertions, 42 deletions

diff --git a/loadtime/src/org/aspectj/weaver/loadtime/Aj.java b/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
index 4884d9eaa..131c13cf0 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/Aj.java
@@ -11,8 +11,13 @@
  *******************************************************************************/
 package org.aspectj.weaver.loadtime;
 
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
 import java.util.Map;
-import java.util.WeakHashMap;
+import java.util.Set;
 
 import org.aspectj.bridge.context.CompilationAndWeavingContext;
 import org.aspectj.weaver.Dump;
@@ -29,7 +34,14 @@ import org.aspectj.weaver.tools.WeavingAdaptor;
 public class Aj implements ClassPreProcessor {
 
 	private IWeavingContext weavingContext;
-	
+
+    /**
+     * References are added to this queue when their associated classloader is removed, and once on here that indicates
+     * that we should tidy up the adaptor map and remove the adaptor (weaver) from the map we are maintaining from
+     * adaptorkey > adaptor (weaver)
+     */
+    private static ReferenceQueue adaptorQueue = new ReferenceQueue();
+
 	private static Trace trace = TraceFactory.getTraceFactory().getTrace(Aj.class);
 	
 	public Aj(){
@@ -96,19 +108,127 @@ public class Aj implements ClassPreProcessor {
     }
 
     /**
+     * An AdaptorKey is a WeakReference wrapping a classloader reference that will enqueue to a specified queue when the
+     * classloader is GC'd. Since the AdaptorKey is used as a key into a hashmap we need to give it a non-varying
+     * hashcode/equals implementation, and we need that hashcode not to vary even when the internal referent has been
+     * GC'd. The hashcode is calculated on creation of the AdaptorKey based on the loader instance that it is wrapping.
+     * This means even when the referent is gone we can still use the AdaptorKey and it will 'point' to the same place
+     * as it always did.
+     */
+    private static class AdaptorKey extends WeakReference {
+
+        private int hashcode = -1;
+
+        public AdaptorKey(ClassLoader loader) {
+            super(loader, adaptorQueue);
+            hashcode = loader.hashCode() * 37;
+        }
+
+        public ClassLoader getClassLoader() {
+            ClassLoader instance = (ClassLoader) get();
+            // Assert instance!=null - shouldn't be asked for after a GC of the referent has occurred !
+            return instance;
+        }
+
+        public boolean equals(Object obj) {
+            if (!(obj instanceof AdaptorKey)) return false;
+            AdaptorKey other = (AdaptorKey) obj;
+            return other.hashcode == hashcode;
+        }
+            
+        public int hashCode() {
+            return hashcode;
+        }
+
+    }
+
+    /**
+     * The reference queue is only processed when a request is made for a weaver adaptor. This means there can be one or
+     * two stale weavers left around. If the user knows they have finished all their weaving, they might wish to call
+     * removeStaleAdaptors which will process anything left on the reference queue containing adaptorKeys for garbage
+     * collected classloaders.
+     * 
+     * @param displayProgress produce System.err info on the tidying up process
+     * @return number of stale weavers removed
+     */
+    public static int removeStaleAdaptors(boolean displayProgress) {
+        int removed = 0;
+        synchronized (WeaverContainer.weavingAdaptors) {
+            if (displayProgress) {
+                System.err.println("Weaver adaptors before queue processing:");
+                Map m = WeaverContainer.weavingAdaptors;
+                Set keys = m.keySet();
+                for (Iterator iterator = keys.iterator(); iterator.hasNext();) {
+                    Object object = (Object) iterator.next();
+                    System.err.println(object + " = " + WeaverContainer.weavingAdaptors.get(object));
+                }
+            }
+            Object o = adaptorQueue.poll();
+            while (o != null) {
+                if (displayProgress) System.err.println("Processing referencequeue entry " + o);
+                AdaptorKey wo = (AdaptorKey) o;
+                boolean didit = WeaverContainer.weavingAdaptors.remove(wo) != null;
+                if (didit) {
+                    removed++;
+                } else {
+                    throw new RuntimeException("Eh?? key="+wo);
+                }
+                if (displayProgress) System.err.println("Removed? "+didit);
+                o = adaptorQueue.poll();
+            }
+            if (displayProgress) {
+                System.err.println("Weaver adaptors after queue processing:");
+                Map m = WeaverContainer.weavingAdaptors;
+                Set keys = m.keySet();
+                for (Iterator iterator = keys.iterator(); iterator.hasNext();) {
+                    Object object = (Object) iterator.next();
+                    System.err.println(object + " = " + WeaverContainer.weavingAdaptors.get(object));
+                }
+            }
+        }
+        return removed;
+    }
+    
+    /**
+     * @return the number of entries still in the weavingAdaptors map
+     */
+    public static int getActiveAdaptorCount() {
+        return WeaverContainer.weavingAdaptors.size();
+    }
+
+    /**
+     * Process the reference queue that contains stale AdaptorKeys - the keys are put on the queue when their
+     * classloader referent is garbage collected and so the associated adaptor (weaver) should be removed from the map
+     */
+    public static void checkQ() {
+        synchronized (adaptorQueue) {
+            Object o = adaptorQueue.poll();
+            while (o != null) {
+                AdaptorKey wo = (AdaptorKey) o;
+                boolean removed = WeaverContainer.weavingAdaptors.remove(wo) != null;
+                // DBG System.err.println("Evicting key " + wo + " = " + didit);
+                o = adaptorQueue.poll();
+            }
+        }
+    }
+
+    /**
      * Cache of weaver
      * There is one weaver per classloader
      */
     static class WeaverContainer {
 
-        private final static Map weavingAdaptors = new WeakHashMap();
+        final static Map weavingAdaptors = Collections.synchronizedMap(new HashMap());
 
         static WeavingAdaptor getWeaver(ClassLoader loader, IWeavingContext weavingContext) {
             ExplicitlyInitializedClassLoaderWeavingAdaptor adaptor = null;
-            synchronized(weavingAdaptors) {
-                adaptor = (ExplicitlyInitializedClassLoaderWeavingAdaptor) weavingAdaptors.get(loader);
+            AdaptorKey adaptorKey = new AdaptorKey(loader);
+
+            synchronized (weavingAdaptors) {
+                checkQ();
+                adaptor = (ExplicitlyInitializedClassLoaderWeavingAdaptor) weavingAdaptors.get(adaptorKey);
                 if (adaptor == null) {
-                	String loaderClassName = loader.getClass().getName(); 
+                    String loaderClassName = loader.getClass().getName(); 
                 	if (loaderClassName.equals("sun.reflect.DelegatingClassLoader")) {
                 		// we don't weave reflection generated types at all! 
                 		return null;
@@ -117,25 +237,13 @@ public class Aj implements ClassPreProcessor {
 	                    // within the synchronized block
 	                    ClassLoaderWeavingAdaptor weavingAdaptor = new ClassLoaderWeavingAdaptor();
 	                    adaptor = new ExplicitlyInitializedClassLoaderWeavingAdaptor(weavingAdaptor);
-	                    weavingAdaptors.put(loader, adaptor);
-                	}
+                        weavingAdaptors.put(adaptorKey, adaptor);
+                    }
                 }
             }
             // perform the initialization
             return adaptor.getWeavingAdaptor(loader, weavingContext);
 
-
-            // old version
-//            synchronized(loader) {//FIXME AV - temp fix for #99861
-//                synchronized (weavingAdaptors) {
-//                    WeavingAdaptor weavingAdaptor = (WeavingAdaptor) weavingAdaptors.get(loader);
-//                    if (weavingAdaptor == null) {
-//                        weavingAdaptor = new ClassLoaderWeavingAdaptor(loader, weavingContext);
-//                        weavingAdaptors.put(loader, weavingAdaptor);
-//                    }
-//                    return weavingAdaptor;
-//                }
-//            }
         }
     }
 
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
index 6c8e6e1d5..628386e0e 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java
@@ -38,6 +38,7 @@ import org.aspectj.weaver.ICrossReferenceHandler;
 import org.aspectj.weaver.Lint;
 import org.aspectj.weaver.ResolvedType;
 import org.aspectj.weaver.UnresolvedType;
+import org.aspectj.weaver.WeakClassLoaderReference;
 import org.aspectj.weaver.World;
 import org.aspectj.weaver.Lint.Kind;
 import org.aspectj.weaver.bcel.BcelWeaver;
@@ -98,25 +99,15 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
     	if (trace.isTraceEnabled()) trace.exit("<init>");
     }
 
-    protected void initialize (final ClassLoader classLoader, IWeavingContext context) {
-    	if (initialized) return;
-
-    	boolean success = true;
-    	if (trace.isTraceEnabled()) trace.enter("initialize",this,new Object[] { classLoader, context });
-
-    	this.weavingContext = context;
-        if (weavingContext == null) {
-        	weavingContext = new DefaultWeavingContext(classLoader);
+   class SimpleGeneratedClassHandler implements GeneratedClassHandler {
+    	private WeakClassLoaderReference loaderRef;
+    	SimpleGeneratedClassHandler(ClassLoader loader) {
+    		loaderRef = new WeakClassLoaderReference(loader);
         }
 
-        createMessageHandler();
-    	
-        this.generatedClassHandler = new GeneratedClassHandler() {
             /**
              * Callback when we need to define a Closure in the JVM
              *
-             * @param name
-             * @param bytes
              */
             public void acceptClass(String name, byte[] bytes) {
                 try {
@@ -127,10 +118,26 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
                     throwable.printStackTrace();
                 }
 
-                defineClass(classLoader, name, bytes);// could be done lazily using the hook
+            defineClass(loaderRef.getClassLoader(), name, bytes); // could be done lazily using the hook
             }
         };
 
+    protected void initialize (final ClassLoader classLoader, IWeavingContext context) {
+    	if (initialized) return;
+
+    	boolean success = true;
+        //    	if (trace.isTraceEnabled()) trace.enter("initialize",this,new Object[] { classLoader, context });
+
+    	this.weavingContext = context;
+        if (weavingContext == null) {
+        	weavingContext = new DefaultWeavingContext(classLoader);
+        }
+
+        createMessageHandler();
+    	
+        this.generatedClassHandler = 
+        	new SimpleGeneratedClassHandler(classLoader);
+
         List definitions = weavingContext.getDefinitions(classLoader,this);
         if (definitions.isEmpty()) {
         	disable(); // TODO maw Needed to ensure messages are flushed
@@ -174,7 +181,7 @@ public class ClassLoaderWeavingAdaptor extends WeavingAdaptor {
      * @param loader
      */
     List parseDefinitions(final ClassLoader loader) {
-        if (trace.isTraceEnabled()) trace.enter("parseDefinitions",this,loader);
+        if (trace.isTraceEnabled()) trace.enter("parseDefinitions", this);
 
         List definitions = new ArrayList();
     	try {
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/DefaultWeavingContext.java b/loadtime/src/org/aspectj/weaver/loadtime/DefaultWeavingContext.java
index 6ffd2cb28..8b9192dbc 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/DefaultWeavingContext.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/DefaultWeavingContext.java
@@ -16,6 +16,7 @@ import java.net.URL;
 import java.util.Enumeration;
 import java.util.List;
 
+import org.aspectj.weaver.WeakClassLoaderReference;
 import org.aspectj.weaver.tools.Trace;
 import org.aspectj.weaver.tools.TraceFactory;
 import org.aspectj.weaver.tools.WeavingAdaptor;
@@ -27,26 +28,26 @@ import org.aspectj.weaver.tools.WeavingAdaptor;
  */
 public class DefaultWeavingContext implements IWeavingContext {
 
-	protected ClassLoader loader;
+	protected WeakClassLoaderReference loaderRef;
 	private String shortName;
 
 	private static Trace trace = TraceFactory.getTraceFactory().getTrace(DefaultWeavingContext.class);
 
 	/**
-	 * Construct a new WeavingContext to use the specifed ClassLoader
+	 * Construct a new WeavingContext to use the specified ClassLoader
 	 * This is the constructor which should be used.
 	 * @param loader
 	 */
 	public DefaultWeavingContext(ClassLoader loader) {
 		super();
-		this.loader = loader;
+		this.loaderRef = new WeakClassLoaderReference(loader);
 	}
 
 	/**
 	 * Same as ClassLoader.getResources()
 	 */
 	public Enumeration getResources(String name) throws IOException {
-		return loader.getResources(name);
+		return getClassLoader().getResources(name);
 	}
 
 	/**
@@ -60,9 +61,15 @@ public class DefaultWeavingContext implements IWeavingContext {
 	 * @return classname@hashcode
 	 */
 	public String getClassLoaderName() {
+		ClassLoader loader = getClassLoader();
     	return ((loader!=null)?loader.getClass().getName()+"@"+Integer.toHexString(System.identityHashCode(loader)):"null");
 	}
 
+
+	public ClassLoader getClassLoader() { 
+		return loaderRef.getClassLoader();
+	}
+
 	/**
 	 * @return filename
 	 */
@@ -88,7 +95,7 @@ public class DefaultWeavingContext implements IWeavingContext {
 
 	public boolean isLocallyDefined(String classname) {
         String asResource = classname.replace('.', '/').concat(".class");
-
+        ClassLoader loader = getClassLoader();
         URL localURL = loader.getResource(asResource);
         if (localURL == null) return false;
 
@@ -108,7 +115,7 @@ public class DefaultWeavingContext implements IWeavingContext {
 	 * @param loader
 	 */
 	public List getDefinitions(final ClassLoader loader, final WeavingAdaptor adaptor) {
-	        if (trace.isTraceEnabled()) trace.enter("getDefinitions",this,new Object[] { loader, adaptor });
+	        if (trace.isTraceEnabled()) trace.enter("getDefinitions", this, new Object[] { "goo", adaptor });
 	
 	        List definitions = ((ClassLoaderWeavingAdaptor)adaptor).parseDefinitions(loader);
 	        
diff --git a/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java b/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java
index 01d3c6d90..10516e46e 100644
--- a/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java
+++ b/loadtime/src/org/aspectj/weaver/loadtime/WeavingURLClassLoader.java
@@ -145,6 +145,7 @@ public class WeavingURLClassLoader extends ExtensibleURLClassLoader implements W
 
 			/* Ensures consistent LTW messages for testing */
 			public String getClassLoaderName() {
+				ClassLoader loader = getClassLoader();
 				return loader.getClass().getName();
 			}
 			
diff --git a/loadtime/testsrc/org/aspectj/weaver/loadtime/WeavingContextTest.java b/loadtime/testsrc/org/aspectj/weaver/loadtime/WeavingContextTest.java
index 97e037317..2e81ca818 100644
--- a/loadtime/testsrc/org/aspectj/weaver/loadtime/WeavingContextTest.java
+++ b/loadtime/testsrc/org/aspectj/weaver/loadtime/WeavingContextTest.java
@@ -152,6 +152,8 @@ public class WeavingContextTest extends TestCase {
 			return "ClassLoaderName";
 		}
 
+		public ClassLoader getClassLoader() { return this.loader;}
+
 		public String getFile(URL url) {
 			return "File";
 		}