Merge pull request quarkusio#47899 from snazy/cl-iae-definePackage

Retry when `CL.definePackage()` throws an IAE

Author: geoand

core/launcher/src/main/java/io/quarkus/launcher/RuntimeLaunchClassLoader.java

@@ -35,14 +35,22 @@ protected Class<?> findClass(String name) throws ClassNotFoundException {
     }
 
     private void definePackage(String name) {
-        final String pkgName = getPackageNameFromClassName(name);
-        if ((pkgName != null) && getPackage(pkgName) == null) {
-            synchronized (getClassLoadingLock(pkgName)) {
-                if (getPackage(pkgName) == null) {
-                    // this could certainly be improved to use the actual manifest
-                    definePackage(pkgName, null, null, null, null, null, null, null);
-                }
+        var pkgName = getPackageNameFromClassName(name);
+        if (pkgName == null) {
+            return;
+        }
+        if (getDefinedPackage(pkgName) != null) {
+            return;
+        }
+        try {
+            // this could certainly be improved to use the actual manifest
+            definePackage(pkgName, null, null, null, null, null, null, null);
+        } catch (IllegalArgumentException e) {
+            // retry, thrown by definePackage(), if a package for the same name is already defines by this class loader.
+            if (getDefinedPackage(pkgName) != null) {
+                return;
             }
+            throw e;
         }
     }
 

independent-projects/bootstrap/core/src/main/java/io/quarkus/bootstrap/classloading/QuarkusClassLoader.java

@@ -27,10 +27,11 @@
 
 import org.jboss.logging.Logger;
 
+import com.google.common.annotations.VisibleForTesting;
+
 import io.quarkus.bootstrap.app.CuratedApplication;
 import io.quarkus.bootstrap.app.StartupAction;
 import io.quarkus.commons.classloading.ClassLoaderHelper;
-import io.quarkus.paths.ManifestAttributes;
 import io.quarkus.paths.PathVisit;
 
 /**
@@ -141,7 +142,6 @@ public static boolean isResourcePresentAtRuntime(String resourcePath) {
     private final List<ClassPathElement> bannedElements;
     private final List<ClassPathElement> parentFirstElements;
     private final ConcurrentMap<ClassPathElement, ProtectionDomain> protectionDomains = new ConcurrentHashMap<>();
-    private final ConcurrentMap<String, Package> definedPackages = new ConcurrentHashMap<>();
     private final ClassLoader parent;
     /**
      * If this is true it will attempt to load from the parent first
@@ -587,28 +587,33 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
         }
     }
 
-    private void definePackage(String name, ClassPathElement classPathElement) {
-        final String pkgName = getPackageNameFromClassName(name);
-        //we can't use getPackage here
-        //if can return a package from the parent
-        if ((pkgName != null) && definedPackages.get(pkgName) == null) {
-            synchronized (getClassLoadingLock(pkgName)) {
-                if (definedPackages.get(pkgName) == null) {
-                    ManifestAttributes manifest = classPathElement.getManifestAttributes();
-                    if (manifest != null) {
-                        definedPackages.put(pkgName, definePackage(pkgName, manifest.getSpecificationTitle(),
-                                manifest.getSpecificationVersion(),
-                                manifest.getSpecificationVendor(),
-                                manifest.getImplementationTitle(),
-                                manifest.getImplementationVersion(),
-                                manifest.getImplementationVendor(), null));
-                        return;
-                    }
-
-                    // this could certainly be improved to use the actual manifest
-                    definedPackages.put(pkgName, definePackage(pkgName, null, null, null, null, null, null, null));
-                }
+    @VisibleForTesting
+    void definePackage(String name, ClassPathElement classPathElement) {
+        var pkgName = getPackageNameFromClassName(name);
+        if (pkgName == null) {
+            return;
+        }
+        if (getDefinedPackage(pkgName) != null) {
+            return;
+        }
+        try {
+            var manifest = classPathElement.getManifestAttributes();
+            if (manifest != null) {
+                definePackage(pkgName, manifest.getSpecificationTitle(),
+                        manifest.getSpecificationVersion(),
+                        manifest.getSpecificationVendor(),
+                        manifest.getImplementationTitle(),
+                        manifest.getImplementationVersion(),
+                        manifest.getImplementationVendor(), null);
+            } else {
+                definePackage(pkgName, null, null, null, null, null, null, null);
+            }
+        } catch (IllegalArgumentException e) {
+            // retry, thrown by definePackage(), if a package for the same name is already defines by this class loader.
+            if (getDefinedPackage(pkgName) != null) {
+                return;
             }
+            throw e;
         }
     }
 

independent-projects/bootstrap/core/src/test/java/io/quarkus/bootstrap/classloading/ConcurrentDefinePackageTestCase.java

@@ -0,0 +1,48 @@
+package io.quarkus.bootstrap.classloading;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.URL;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
+
+import org.junit.jupiter.api.Test;
+
+public class ConcurrentDefinePackageTestCase {
+
+    /**
+     * Validates that {@link QuarkusClassLoader} can safely do concurrent class loading against "new" packages,
+     * backed by {@link ClassLoader#definePackage(String, String, String, String, String, String, String, URL)}, which throws an
+     * {@link IllegalArgumentException} if the package is already defined.
+     */
+    @Test
+    public void concurrentDefinePackage() throws Exception {
+        var threads = Math.max(8, Runtime.getRuntime().availableProcessors() * 2);
+        var pool = Executors.newFixedThreadPool(threads);
+        try (var quarkusClassLoader = QuarkusClassLoader.builder("test", Thread.currentThread().getContextClassLoader(), false)
+                .build()) {
+            for (var pkg = 0; pkg < 200; pkg++) {
+                var readyLatch = new CountDownLatch(threads);
+                var startLatch = new CountDownLatch(1);
+                var pkgName = "my.package" + pkg;
+                var futures = IntStream.range(0, threads).mapToObj(i -> CompletableFuture.runAsync(() -> {
+                    readyLatch.countDown();
+                    try {
+                        assertThat(startLatch.await(5, TimeUnit.MINUTES)).isTrue();
+                    } catch (InterruptedException e) {
+                        throw new RuntimeException(e);
+                    }
+                    quarkusClassLoader.definePackage(pkgName, ClassPathElement.EMPTY);
+                }, pool)).toArray(CompletableFuture[]::new);
+                assertThat(readyLatch.await(5, TimeUnit.MINUTES)).isTrue();
+                startLatch.countDown();
+                CompletableFuture.allOf(futures).get(5, TimeUnit.MINUTES);
+            }
+        } finally {
+            pool.shutdown();
+        }
+    }
+}