Merge pull request #1409 from hcoles/bug/quarkus_classloader_leak

Support quarkus 3.22.2 and above

Author: hcoles

pitest-maven-verification/src/test/java/org/pitest/PitMojoIT.java

@@ -380,23 +380,48 @@ public void shouldWorkWithGWTMockito() throws Exception {
   @Test
   // note this test depends on the junit5 plugin
   public void shouldWorkWithQuarkus() throws Exception {
-    assumeTrue(CurrentRuntime.version() >= 11);
+    assumeTrue(CurrentRuntime.version() >= 17);
 
     File testDir = prepare("/pit-quarkus");
     verifier.executeGoal("test");
     verifier.executeGoal("org.pitest:pitest-maven:mutationCoverage");
 
     String actual = readResults(testDir);
+
+    assertThat(actual)
+            .contains(
+                    "<mutation detected='true' status='KILLED' numberOfTestsRun='1'><sourceFile>ExampleController.java</sourceFile>");
+
+    assertThat(actual)
+            .contains(
+                    "status='SURVIVED'");
+
     // Test is flaky. Needs investigation
     //assertThat(actual)
     //        .contains(
     //                "<mutation detected='false' status='SURVIVED' numberOfTestsRun='2'>" +
     //                        "<sourceFile>ExampleController.java</sourceFile>");
 
+  }
+
+  @Test
+  public void shouldWorkWithOlderQuarkusVersions() throws Exception {
+    assumeTrue(CurrentRuntime.version() >= 17);
+
+    File testDir = prepare("/pit-quarkus", "-Dquarkus.platform.version=3.21.4");
+    verifier.executeGoal("test");
+    verifier.executeGoal("org.pitest:pitest-maven:mutationCoverage");
+
+    String actual = readResults(testDir);
+
     assertThat(actual)
             .contains(
                     "<mutation detected='true' status='KILLED' numberOfTestsRun='1'><sourceFile>ExampleController.java</sourceFile>");
 
+    assertThat(actual)
+            .contains(
+                    "status='SURVIVED'");
+
   }
 
 
@@ -517,7 +542,7 @@ private static String readCoverage(File testDir) throws IOException {
     return FileUtils.readFileToString(coverage);
   }
 
-  private File prepare(String testPath) throws IOException,
+  private File prepare(String testPath, String ... options) throws IOException,
           VerificationException {
     String path = ResourceExtractor.extractResourcePath(getClass(), testPath,
             testFolder.getRoot(), true).getAbsolutePath();
@@ -527,6 +552,11 @@ private File prepare(String testPath) throws IOException,
     verifier.setDebug(true);
     verifier.getCliOptions().add("-Dverbose=true");
     verifier.getCliOptions().add("-Dpit.version=" + VERSION);
+
+    for (String option : options) {
+      verifier.getCliOptions().add(option);
+    }
+
     verifier.getCliOptions().add(
             "-Dthreads=" + (Runtime.getRuntime().availableProcessors()));
 

pitest-maven-verification/src/test/resources/pit-quarkus/pom.xml

@@ -12,9 +12,9 @@
     <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
     <quarkus.platform.artifact-id>quarkus-bom</quarkus.platform.artifact-id>
     <quarkus.platform.group-id>io.quarkus.platform</quarkus.platform.group-id>
-    <quarkus.platform.version>3.4.3</quarkus.platform.version>
+    <quarkus.platform.version>3.22.3</quarkus.platform.version>
     <surefire-plugin.version>3.1.2</surefire-plugin.version>
-    <pitest.junit5.version>1.0.0</pitest.junit5.version>
+    <pitest.junit5.version>1.2.3</pitest.junit5.version>
   </properties>
   <dependencyManagement>
     <dependencies>
@@ -28,9 +28,9 @@
     </dependencies>
   </dependencyManagement>
   <dependencies>
-   <dependency>
+    <dependency>
       <groupId>io.quarkus</groupId>
-      <artifactId>quarkus-resteasy-reactive</artifactId>
+      <artifactId>quarkus-rest</artifactId>
     </dependency>
     <dependency>
       <groupId>io.quarkus</groupId>
@@ -47,10 +47,7 @@
       <artifactId>rest-assured</artifactId>
       <scope>test</scope>
     </dependency>
-    <dependency>
-      <groupId>io.quarkus</groupId>
-      <artifactId>quarkus-resteasy-reactive-jackson</artifactId>
-    </dependency>
+
   </dependencies>
   <build>
     <plugins>

pitest-maven-verification/src/test/resources/pit-quarkus/src/main/java/com/example/service/ExampleService.java

@@ -7,6 +7,7 @@
 public class ExampleService {
 
     public boolean doStuff(String s) {
+        System.out.println("this survives");
         return s.equals("foo");
     }
 

pitest/src/main/java/org/pitest/coverage/execute/CoverageMinion.java

@@ -164,10 +164,12 @@ private static List<TestUnit> getTestsFromParent(
   private static List<TestUnit> discoverTests(Configuration testPlugin, List<ClassName> classes, CoveragePipe invokeQueue) {
     TestUnitExecutionListener listener = new CoverageTestExecutionListener(invokeQueue);
     FindTestUnits finder = new FindTestUnits(testPlugin, listener);
+
     List<TestUnit> tus = finder
         .findTestUnitsForAllSuppliedClasses(classes.stream()
                 .flatMap(ClassName.nameToClass())
                 .collect(Collectors.toList()));
+
     LOG.info(() -> "Found " + tus.size() + " tests");
     return tus;
   }

pitest/src/main/java/org/pitest/mutationtest/execute/CatchNewClassLoadersTransformer.java

@@ -10,6 +10,7 @@
 import java.util.Map;
 import java.util.WeakHashMap;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 /**
  * Pitest mainly inserts mutants by calling Instrumentation.redefineClasses using
@@ -35,8 +36,14 @@ public class CatchNewClassLoadersTransformer implements ClassFileTransformer {
     private static String targetClass;
     private static byte[] currentMutant;
 
+    //
+    // Storage was introduced to support Quarkus, however they changed their classloading strategy in
+    // 3.22.2 so it is no longer necessary. As storage runs the risk of performance degradation if too
+    // many classloaders are encountered removing this shouldn be investigated once older versions of
+    // quarkus are out of circulation.
+    //
     // What we want is a ConcurrentWeakHasSet, since that doesn't exist without writing one ourselves
-    // we'll abuse a WeakHashMap and live with the synchronization
+    // we'll abuse a WeakHashMap and live with the synchronization.
     static final Map<ClassLoader, Object> CLASS_LOADERS = Collections.synchronizedMap(new WeakHashMap<>());
 
     public static synchronized void setMutant(String className, byte[] mutant) {
@@ -59,7 +66,9 @@ public byte[] transform(final ClassLoader loader, final String className,
                             final ProtectionDomain protectionDomain, final byte[] classfileBuffer) {
 
         if (className.equals(targetClass) && shouldTransform(loader)) {
-            CLASS_LOADERS.put(loader, null);
+            if (shouldStore(loader)) {
+                CLASS_LOADERS.put(loader, null);
+            }
             // we might be mid-mutation so return the mutated bytes
             return currentMutant;
         }
@@ -84,16 +93,28 @@ private static Class<?> checkClassForLoader(ClassLoader loader, String className
 
     }
 
+    private boolean shouldStore(ClassLoader loader) {
+        // quarkus generates large numbers of ParentLastURLClassLoaders. They need to be
+        // transformed when first encountered, but do not seem to be used to load the mutated
+        // class in subsequent tests. Re-transforming them would introduce a significant performance hit.
+        return !loader.getClass().getName().equals("io.quarkus.test.junit.classloading.ParentLastURLClassLoader");
+    }
+
     private boolean shouldTransform(ClassLoader loader) {
         // Only gwtmockito has been identified so far as a loader not to transform
         // but there will be others
         return !loader.getClass().getName().startsWith("com.google.gwtmockito.");
     }
 
     private static void logClassloaders() {
-        if (CLASS_LOADERS.size() > 1) {
+        int count = CLASS_LOADERS.size();
+        if (count > 30) {
+            LOG.warning("Accumulated " + CLASS_LOADERS.size() + " classloaders, this will likely degrade performance. Please report this as an issue.");
+            LOG.warning("Accumulated classloaders = " + CLASS_LOADERS.keySet().stream().map(Object::toString).collect(Collectors.joining("\n")));
+        } else if (count > 1) {
             LOG.fine("Accumulated " + CLASS_LOADERS.size() + " classloaders");
         }
+
     }
 
 }