Fix LTW with a parallel-capable class loader and generated classes

loadtime/src/main/java/org/aspectj/weaver/loadtime/ClassLoaderWeavingAdaptor.java

@@ -1008,21 +1008,8 @@ public void flushGeneratedClasses() {
 	 * @param className a slashed classname (e.g. com/foo/Bar)
 	 */
 	public void flushGeneratedClassesFor(String className) {
-		try {
-			String dottedClassName = className.replace('/', '.');
-			String dottedClassNameDollar = dottedClassName+"$"; // to pickup inner classes
-			Iterator<Map.Entry<String, IUnwovenClassFile>> iter = generatedClasses.entrySet().iterator();
-			while (iter.hasNext()) {
-				Entry<String, IUnwovenClassFile> next = iter.next();
-				String existingGeneratedName = next.getKey();
-				if (existingGeneratedName.equals(dottedClassName) ||
-						existingGeneratedName.startsWith(dottedClassNameDollar)) {
-					iter.remove();
-				}
-			}
-		} catch (Throwable t) {
-			new RuntimeException("Unexpected problem tidying up generated classes for "+className,t).printStackTrace();
-		}
+		String dottedClassName = className.replace('/', '.');
+		generatedClasses.remove(dottedClassName);
 	}
 
 	private static final Object lock = new Object();

tests/bugs1921/github_279/Application.java

@@ -0,0 +1,44 @@
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+/**
+ * https://github.com/eclipse-aspectj/aspectj/issues/279
+ */
+public class Application {
+  public static AtomicInteger HELLO_COUNT = new AtomicInteger(0);
+  public static AtomicInteger ASPECT_COUNT = new AtomicInteger(0);
+
+  private static final int ROUNDS = 25;
+  private static final int THREAD_COUNT = 2;
+  private static final int TOTAL_COUNT = ROUNDS * THREAD_COUNT;
+  private static final String CLASS_TO_LOAD = "GreeterImpl";
+
+  public static void main(String[] args) throws Exception {
+    for (int round = 0; round < ROUNDS; round++) {
+      ExecutorService executor = Executors.newFixedThreadPool(THREAD_COUNT);
+      ParallelCapableClassLoader cl = new ParallelCapableClassLoader(Application.class.getClassLoader(), CLASS_TO_LOAD);
+      for (int i = 0; i < THREAD_COUNT; i++) {
+        executor.submit(() -> {
+          try {
+            Class<?> myClass = Class.forName(CLASS_TO_LOAD, true, cl);
+            Greeter greeter = (Greeter) myClass.getConstructor(new Class<?>[] {}).newInstance();
+            greeter.hello();
+            HELLO_COUNT.incrementAndGet();
+          }
+          catch (Exception e) {
+            throw new RuntimeException(e);
+          }
+        });
+      }
+      executor.shutdown();
+      executor.awaitTermination(60, TimeUnit.SECONDS);
+    }
+
+    assert HELLO_COUNT.get() == TOTAL_COUNT
+      : String.format("Hello count should be %s, but is %s", TOTAL_COUNT, HELLO_COUNT.get());
+    assert ASPECT_COUNT.get() == TOTAL_COUNT
+      : String.format("Aspect count should be %s, but is %s", TOTAL_COUNT, ASPECT_COUNT.get());
+  }
+}

tests/bugs1921/github_279/Greeter.java

@@ -0,0 +1,3 @@
+public interface Greeter {
+  String hello();
+}

tests/bugs1921/github_279/GreeterImpl.java

@@ -0,0 +1,6 @@
+public class GreeterImpl implements Greeter {
+  @Override
+  public String hello() {
+    return "World!";
+  }
+}

tests/bugs1921/github_279/HelloInterceptor.java

@@ -0,0 +1,12 @@
+import org.aspectj.lang.ProceedingJoinPoint;
+import org.aspectj.lang.annotation.Around;
+import org.aspectj.lang.annotation.Aspect;
+
+@Aspect
+public class HelloInterceptor {
+  @Around("execution(public String Greeter.hello())")
+  public Object interceptHello(ProceedingJoinPoint pjp) throws Throwable {
+    Application.ASPECT_COUNT.incrementAndGet();
+    return pjp.proceed();
+  }
+}

tests/bugs1921/github_279/ParallelCapableClassLoader.java

@@ -0,0 +1,56 @@
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+
+public class ParallelCapableClassLoader extends ClassLoader {
+  private final ClassLoader delegate;
+  private final String classNameToHandle;
+
+  static {
+    if (!ClassLoader.registerAsParallelCapable())
+      throw new RuntimeException("Failed to register " + ParallelCapableClassLoader.class.getName() + " as parallel-capable");
+  }
+
+  public ParallelCapableClassLoader(ClassLoader delegate, String classNameToHandle) {
+    this.delegate = delegate;
+    this.classNameToHandle = classNameToHandle;
+  }
+
+  @Override
+  protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
+    Class<?> c = this.findLoadedClass(name);
+    if (c == null && name.equals(classNameToHandle)) {
+      byte[] bytes = getClassBytes(name);
+      try {
+        c = defineClass(name, bytes, 0, bytes.length);
+      }
+      catch (LinkageError e) {
+        c = findLoadedClass(name);
+        if (c == null)
+          throw e;
+      }
+    }
+    if (c == null)
+      c = delegate.loadClass(name);
+    if (resolve)
+      this.resolveClass(c);
+    return c;
+  }
+
+  private byte[] getClassBytes(String name) {
+    String classFilePath = name.replace('.', File.separatorChar) + ".class";
+    try (InputStream inputStream = delegate.getResourceAsStream(classFilePath)) {
+      ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+      int bytesRead;
+      byte[] buffer = new byte[4096];
+      while ((bytesRead = inputStream.read(buffer)) != -1) {
+        outputStream.write(buffer, 0, bytesRead);
+      }
+      return outputStream.toByteArray();
+    }
+    catch (IOException e) {
+      throw new RuntimeException(e);
+    }
+  }
+}

tests/bugs1921/github_279/ant.xml

@@ -0,0 +1,15 @@
+<!-- ajc-ant script, not to be used from Ant commant line - see AntSpec -->
+<project name="ltw">
+  <target name="same class woven concurrently in parallel-capable classloader">
+    <copy file="${aj.root}/tests/bugs1921/github_279/aop.xml" tofile="${aj.sandbox}/META-INF/aop.xml"/>
+    <java fork="yes" classname="Application" failonerror="yes">
+      <classpath refid="aj.path"/>
+      <jvmarg value="-ea"/>
+      <!-- use META-INF/aop.xml style -->
+      <jvmarg value="-javaagent:${aj.root}/lib/test/loadtime5.jar"/>
+      <!--<jvmarg value="${aj.addOpensKey}"/>-->
+      <!--<jvmarg value="${aj.addOpensValue}"/>-->
+    </java>
+  </target>
+
+</project>

tests/bugs1921/github_279/aop.xml

@@ -0,0 +1,5 @@
+<aspectj>
+  <aspects>
+    <aspect name="HelloInterceptor"/>
+  </aspects>
+</aspectj>

tests/src/test/java/org/aspectj/systemtest/ajc1921/Bugs1921Tests.java

@@ -15,8 +15,8 @@
  */
 public class Bugs1921Tests extends XMLBasedAjcTestCase {
 
-  public void testDummy() {
-    //runTest("dummy");
+  public void testGitHub_279() {
+    runTest("same class woven concurrently in parallel-capable classloader");
   }
 
   public static Test suite() {

tests/src/test/resources/org/aspectj/systemtest/ajc1921/ajc1921.xml

@@ -333,4 +333,12 @@
 		</run>
 	</ajc-test>
 
+	<!-- https://github.com/eclipse-aspectj/aspectj/issues/279 -->
+	<ajc-test dir="bugs1921/github_279" vm="8" title="same class woven concurrently in parallel-capable classloader">
+		<compile files="Application.java Greeter.java GreeterImpl.java ParallelCapableClassLoader.java" options="-8"/>
+		<compile files="HelloInterceptor.java" options="-8 -Xlint:ignore"/>
+		<!-- Problem only reproduces in forked JVM with java agent, hence Ant build -->
+		<ant file="ant.xml" target="same class woven concurrently in parallel-capable classloader" verbose="true"/>
+	</ajc-test>
+
 </suite>

weaver/src/main/java/org/aspectj/weaver/bcel/BcelWeaver.java

@@ -972,29 +972,7 @@ public Iterator<UnwovenClassFile> getClassFileIterator() {
 			}
 
 			public IWeaveRequestor getRequestor() {
-				return new IWeaveRequestor() {
-					public void acceptResult(IUnwovenClassFile result) {
-						try {
-							writeZipEntry(result.getFilename(), result.getBytes());
-						} catch (IOException ex) {
-						}
-					}
-
-					public void processingReweavableState() {
-					}
-
-					public void addingTypeMungers() {
-					}
-
-					public void weavingAspects() {
-					}
-
-					public void weavingClasses() {
-					}
-
-					public void weaveCompleted() {
-					}
-				};
+				return new WeaveRequestor();
 			}
 		});
 		// /* BUG 40943*/
@@ -1003,6 +981,25 @@ public void weaveCompleted() {
 		return c;
 	}
 
+	private class WeaveRequestor implements IWeaveRequestor {
+		public void acceptResult(IUnwovenClassFile result) {
+			try {
+				writeZipEntry(result.getFilename(), result.getBytes());
+			} catch (IOException ex) {
+			}
+		}
+
+		public void processingReweavableState() {}
+
+		public void addingTypeMungers() {}
+
+		public void weavingAspects() {}
+
+		public void weavingClasses() {}
+
+		public void weaveCompleted() {}
+	}
+
 	private Set<IProgramElement> candidatesForRemoval = null;
 
 	// variation of "weave" that sources class files from an external source.

weaver/src/main/java/org/aspectj/weaver/tools/WeavingAdaptor.java

@@ -358,10 +358,18 @@ public byte[] weaveClass(String name, final byte[] bytes, boolean mustWeave) thr
 
 			boolean debugOn = !messageHandler.isIgnoring(Message.DEBUG);
 
+			name = name.replace('/', '.');
+			byte[] wovenBytes = wovenWithGeneratedClass(name);
+			if (wovenBytes != null) {
+				if (debugOn) {
+					debug("returning woven bytes for '" + name + "' that were generated by a previous weaving process");
+				}
+				return wovenBytes;
+			}
+
 			try {
 				delegateForCurrentClass = null;
-				name = name.replace('/', '.');
-				if (couldWeave(name, bytes)) {
+				if (shouldWeaveName(name)) {
 					if (accept(name, bytes)) {
 
 						// Determine if we have the weaved class cached
@@ -441,11 +449,18 @@ public byte[] weaveClass(String name, final byte[] bytes, boolean mustWeave) thr
 	}
 
 	/**
+	 * Return the bytes from a (parallel?) weaving process that generated an inner class, e.g. to support Around closures.
+	 * This is done instead of weaving again, as weaving would generate another inner class.
 	 * @param name
-	 * @return true if even valid to weave: either with an accept check or to munge it for @AspectJ aspectof support
+	 * @return the cached bytes of a previously woven class, or null if not found
 	 */
-	private boolean couldWeave(String name, byte[] bytes) {
-		return !generatedClasses.containsKey(name) && shouldWeaveName(name);
+	private byte[] wovenWithGeneratedClass(String name) {
+		IUnwovenClassFile woven = generatedClasses.get(name);
+		if (woven == null) {
+			return null;
+		}
+
+		return woven.getBytes();
 	}
 
 	// ATAJ
@@ -901,53 +916,50 @@ public Iterator<UnwovenClassFile> getClassFileIterator() {
 		}
 
 		public IWeaveRequestor getRequestor() {
-			return new IWeaveRequestor() {
-
-				public void acceptResult(IUnwovenClassFile result) {
-					if (wovenClass == null) {
-						wovenClass = result;
-						String name = result.getClassName();
-						if (shouldDump(name.replace('/', '.'), false)) {
-							dump(name, result.getBytes(), false);
-						}
-					} else {
-						// Classes generated by weaver e.g. around closure advice
-						String className = result.getClassName();
-						byte[]	resultBytes = result.getBytes();
-
-						if (SimpleCacheFactory.isEnabled()) {
-							SimpleCache lacache=SimpleCacheFactory.createSimpleCache();
-							lacache.put(result.getClassName(), wovenClass.getBytes(), result.getBytes());
-							lacache.addGeneratedClassesNames(wovenClass.getClassName(), wovenClass.getBytes(), result.getClassName());
-						}
+			return new WeaveRequestor();
+		}
 
-						generatedClasses.put(className, result);
-						generatedClasses.put(wovenClass.getClassName(), result);
-						generatedClassHandler.acceptClass(className, null, resultBytes);
+		private class WeaveRequestor implements IWeaveRequestor {
+			@Override
+			public void acceptResult(IUnwovenClassFile result) {
+				if (wovenClass == null) {
+					wovenClass = result;
+					String name = result.getClassName();
+					if (shouldDump(name.replace('/', '.'), false)) {
+						dump(name, result.getBytes(), false);
+					}
+				} else {
+					// Classes generated by weaver e.g. around closure advice
+					String className = result.getClassName();
+					byte[]	resultBytes = result.getBytes();
+
+					if (SimpleCacheFactory.isEnabled()) {
+						SimpleCache lacache=SimpleCacheFactory.createSimpleCache();
+						lacache.put(result.getClassName(), wovenClass.getBytes(), result.getBytes());
+						lacache.addGeneratedClassesNames(wovenClass.getClassName(), wovenClass.getBytes(), result.getClassName());
 					}
-				}
 
-				public void processingReweavableState() {
+					generatedClasses.put(wovenClass.getClassName(), wovenClass);
+					generatedClassHandler.acceptClass(className, null, resultBytes);
 				}
+			}
 
-				public void addingTypeMungers() {
-				}
+			public void processingReweavableState() {}
 
-				public void weavingAspects() {
-				}
+			public void addingTypeMungers() {}
 
-				public void weavingClasses() {
-				}
+			public void weavingAspects() {}
 
-				public void weaveCompleted() {
-					// ResolvedType.resetPrimitives();
-					if (delegateForCurrentClass != null) {
-						delegateForCurrentClass.weavingCompleted();
-					}
-					// ResolvedType.resetPrimitives();
-					// bcelWorld.discardType(typeBeingProcessed.getResolvedTypeX()); // work in progress
+			public void weavingClasses() {}
+
+			public void weaveCompleted() {
+				// ResolvedType.resetPrimitives();
+				if (delegateForCurrentClass != null) {
+					delegateForCurrentClass.weavingCompleted();
 				}
-			};
+				// ResolvedType.resetPrimitives();
+				// bcelWorld.discardType(typeBeingProcessed.getResolvedTypeX()); // work in progress
+			}
 		}
 	}