Do not use cached classpath elements when initializing deployment and

runtime classloaders

Author: aloubyansky

independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java

@@ -3,7 +3,7 @@
 import java.io.IOException;
 import java.io.Serializable;
 import java.nio.file.Path;
-import java.nio.file.Paths;
+import java.util.Objects;
 
 public class DirectoryPathTree extends OpenContainerPathTree implements Serializable {
 
@@ -28,12 +28,12 @@ public DirectoryPathTree(Path dir, PathFilter pathFilter) {
 
     public DirectoryPathTree(Path dir, PathFilter pathFilter, boolean manifestEnabled) {
         super(pathFilter, manifestEnabled);
-        this.dir = dir;
+        this.dir = Objects.requireNonNull(dir, "Directory is null");
     }
 
     protected DirectoryPathTree(Path dir, PathFilter pathFilter, PathTreeWithManifest pathTreeWithManifest) {
         super(pathFilter, pathTreeWithManifest);
-        this.dir = dir;
+        this.dir = Objects.requireNonNull(dir, "Directory is null");
     }
 
     @Override
@@ -72,7 +72,7 @@ private void writeObject(java.io.ObjectOutputStream out) throws IOException {
     }
 
     private void readObject(java.io.ObjectInputStream in) throws IOException, ClassNotFoundException {
-        dir = Paths.get(in.readUTF());
+        dir = Path.of(in.readUTF());
         pathFilter = (PathFilter) in.readObject();
         manifestEnabled = in.readBoolean();
     }

independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/OpenContainerPathTree.java

@@ -93,7 +93,11 @@ public OpenPathTree open() {
 
     @Override
     public Collection<Path> getRoots() {
-        return List.of(getRootPath());
+        final Path rootPath = getRootPath();
+        if (rootPath == null) {
+            throw new RuntimeException("rootPath is null for " + getContainerPath());
+        }
+        return List.of(rootPath);
     }
 
     @Override

independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/SharedArchivePathTree.java

@@ -53,8 +53,11 @@ static void removeFromCache(Path path) {
     @Override
     public OpenPathTree open() {
         var lastOpen = this.lastOpen;
-        if (lastOpen != null && lastOpen.acquire()) {
-            return new CallerOpenPathTree(lastOpen);
+        if (lastOpen != null) {
+            var acquired = lastOpen.acquire();
+            if (acquired != null) {
+                return acquired;
+            }
         }
         try {
             this.lastOpen = new SharedOpenArchivePathTree(openFs());
@@ -73,14 +76,28 @@ protected SharedOpenArchivePathTree(FileSystem fs) {
             openCount.incrementAndGet();
         }
 
-        private boolean acquire() {
+        /**
+         * Returns a new handle for this open archive tree to the caller
+         * as long as this open archive tree is still open and is still
+         * the last one that was open for this archive. Otherwise, the method
+         * will return null.
+         *
+         * @return a new instance of {@link CallerOpenPathTree} or null,
+         *         if the current open archive tree has been closed or another open
+         *         archive tree has been created for this archive
+         */
+        private CallerOpenPathTree acquire() {
             readLock().lock();
-            final boolean result = lastOpen == this && isOpen();
-            if (result) {
-                users.incrementAndGet();
+            try {
+                final boolean result = lastOpen == this && isOpen();
+                if (result) {
+                    users.incrementAndGet();
+                    return new CallerOpenPathTree(this);
+                }
+            } finally {
+                readLock().unlock();
             }
-            readLock().unlock();
-            return result;
+            return null;
         }
 
         @Override

independent-projects/bootstrap/core/src/main/java/io/quarkus/bootstrap/app/CuratedApplication.java

@@ -146,7 +146,22 @@ private Object runInCl(String consumerName, Map<String, Object> params, QuarkusC
         }
     }
 
-    private synchronized void processCpElement(ResolvedDependency artifact, Consumer<ClassPathElement> consumer) {
+    /**
+     * Turns a resolved dependency into a classpath element and calls a consumer to process it.
+     * The consumer will add the classpath element to a {@link QuarkusClassLoader} instance.
+     *
+     * TODO: useCpeCache argument is a quick workaround to avoid the risk of sharing classpath elements
+     * among multiple instances of deployment and runtime classloaders. When these shared classpath elements
+     * happen to be JARs, when the first deployment classloader is closed, its JAR classpath elements will
+     * get closed as well but will remain cached, so the next deployment classloader will be initialized
+     * with the closed JAR classpath elements.
+     *
+     * @param artifact resolved dependency
+     * @param consumer classpath element consumer
+     * @param useCpeCache whether to re-use cached classpath elements
+     */
+    private synchronized void processCpElement(ResolvedDependency artifact, Consumer<ClassPathElement> consumer,
+            boolean useCpeCache) {
         if (!artifact.isJar()) {
             //avoid the need for this sort of check in multiple places
             consumer.accept(ClassPathElement.EMPTY);
@@ -162,7 +177,7 @@ public void accept(ClassPathElement classPathElement) {
                 }
             };
         }
-        ClassPathElement cpe = augmentationElements.get(artifact.getKey());
+        ClassPathElement cpe = useCpeCache ? augmentationElements.get(artifact.getKey()) : null;
         if (cpe != null) {
             consumer.accept(cpe);
             return;
@@ -172,9 +187,11 @@ public void accept(ClassPathElement classPathElement) {
             consumer.accept(ClassPathElement.EMPTY);
             return;
         }
-        cpe = ClassPathElement.fromDependency(artifact);
+        cpe = ClassPathElement.fromDependency(contentTree, artifact);
         consumer.accept(cpe);
-        augmentationElements.put(artifact.getKey(), cpe);
+        if (useCpeCache) {
+            augmentationElements.put(artifact.getKey(), cpe);
+        }
     }
 
     private void addCpElement(QuarkusClassLoader.Builder builder, ResolvedDependency dep, ClassPathElement element) {
@@ -204,13 +221,13 @@ public synchronized QuarkusClassLoader getOrCreateAugmentClassLoader() {
             //this will load any deployment artifacts from the parent CL if they are present
             for (ResolvedDependency i : appModel.getDependencies()) {
                 if (configuredClassLoading.isRemovedArtifact(i.getKey())) {
-                    processCpElement(i, builder::addBannedElement);
+                    processCpElement(i, builder::addBannedElement, true);
                     continue;
                 }
                 if (configuredClassLoading.isReloadableArtifact(i.getKey())) {
                     continue;
                 }
-                processCpElement(i, element -> addCpElement(builder, i, element));
+                processCpElement(i, element -> addCpElement(builder, i, element), true);
             }
 
             for (Path i : quarkusBootstrap.getAdditionalDeploymentArchives()) {
@@ -297,7 +314,7 @@ public synchronized QuarkusClassLoader getOrCreateBaseRuntimeClassLoader() {
 
             for (ResolvedDependency dependency : appModel.getDependencies()) {
                 if (configuredClassLoading.isRemovedArtifact(dependency.getKey())) {
-                    processCpElement(dependency, builder::addBannedElement);
+                    processCpElement(dependency, builder::addBannedElement, true);
                     continue;
                 }
                 if (!dependency.isRuntimeCp()
@@ -307,7 +324,7 @@ public synchronized QuarkusClassLoader getOrCreateBaseRuntimeClassLoader() {
                                 && appModel.getReloadableWorkspaceDependencies().contains(dependency.getKey())) {
                     continue;
                 }
-                processCpElement(dependency, element -> addCpElement(builder, dependency, element));
+                processCpElement(dependency, element -> addCpElement(builder, dependency, element), true);
             }
 
             baseRuntimeClassLoader = builder.build();
@@ -358,7 +375,7 @@ public QuarkusClassLoader createDeploymentClassLoader() {
                 continue;
             }
             if (isReloadableRuntimeDependency(dependency)) {
-                processCpElement(dependency, element -> addCpElement(builder, dependency, element));
+                processCpElement(dependency, element -> addCpElement(builder, dependency, element), false);
             }
         }
         for (Path root : configuredClassLoading.getAdditionalClasspathElements()) {
@@ -389,7 +406,7 @@ public QuarkusClassLoader createRuntimeClassLoader(ClassLoader base, Map<String,
                                 + getClassLoaderNameSuffix()
                                 + " restart no:"
                                 + runtimeClassLoaderCount.getAndIncrement(),
-                        getOrCreateBaseRuntimeClassLoader(), false)
+                        base, false)
                 .setAssertionsEnabled(quarkusBootstrap.isAssertionsEnabled())
                 .setCuratedApplication(this)
                 .setAggregateParentResources(true);
@@ -412,7 +429,7 @@ public QuarkusClassLoader createRuntimeClassLoader(ClassLoader base, Map<String,
                 continue;
             }
             if (isReloadableRuntimeDependency(dependency)) {
-                processCpElement(dependency, element -> addCpElement(builder, dependency, element));
+                processCpElement(dependency, element -> addCpElement(builder, dependency, element), false);
             }
         }
         for (Path root : configuredClassLoading.getAdditionalClasspathElements()) {

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

@@ -102,10 +102,14 @@ static ClassPathElement fromPath(Path path, boolean runtime) {
     }
 
     static ClassPathElement fromDependency(ResolvedDependency dep) {
-        return new PathTreeClassPathElement(dep.getContentTree(), dep.isRuntimeCp(), dep);
+        return fromDependency(dep.getContentTree(), dep);
     }
 
-    static ClassPathElement EMPTY = new ClassPathElement() {
+    static ClassPathElement fromDependency(PathTree contentTree, ResolvedDependency dep) {
+        return new PathTreeClassPathElement(contentTree, dep.isRuntimeCp(), dep);
+    }
+
+    ClassPathElement EMPTY = new ClassPathElement() {
 
         @Override
         public Path getRoot() {

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

@@ -28,7 +28,6 @@
 import io.quarkus.paths.OpenPathTree;
 import io.quarkus.paths.PathTree;
 import io.quarkus.paths.PathVisit;
-import io.quarkus.paths.PathVisitor;
 
 public class PathTreeClassPathElement extends AbstractClassPathElement {
 
@@ -104,7 +103,11 @@ public ClassPathResource getResource(String name) {
         if (resources != null && !resources.contains(sanitized)) {
             return null;
         }
-        return apply(tree -> tree.apply(sanitized, visit -> visit == null ? null : new Resource(visit)));
+        return apply(tree -> tree.apply(sanitized, this::getResource));
+    }
+
+    private Resource getResource(PathVisit visit) {
+        return visit == null ? null : new Resource(visit);
     }
 
     @Override
@@ -114,17 +117,15 @@ public List<ClassPathResource> getResources(String name) {
         if (resources != null && !resources.contains(sanitized)) {
             return List.of();
         }
-        List<ClassPathResource> ret = new ArrayList<>();
-        apply(tree -> {
+        return apply(tree -> {
+            final List<ClassPathResource> ret = new ArrayList<>();
             tree.acceptAll(sanitized, visit -> {
                 if (visit != null) {
                     ret.add(new Resource(visit));
-
                 }
             });
-            return List.of();
+            return ret;
         });
-        return ret;
     }
 
     @Override
@@ -155,25 +156,23 @@ public <T> T apply(Function<OpenPathTree, T> func) {
     public Set<String> getProvidedResources() {
         Set<String> resources = this.resources;
         if (resources == null) {
-            resources = apply(tree -> {
-                final Set<String> relativePaths = new HashSet<>();
-                tree.walk(new PathVisitor() {
-                    @Override
-                    public void visitPath(PathVisit visit) {
-                        final String relativePath = visit.getRelativePath("/");
-                        if (relativePath.isEmpty()) {
-                            return;
-                        }
-                        relativePaths.add(relativePath);
-                    }
-                });
-                return relativePaths;
-            });
+            resources = apply(PathTreeClassPathElement::collectResources);
             this.resources = resources;
         }
         return resources;
     }
 
+    private static Set<String> collectResources(OpenPathTree tree) {
+        final Set<String> relativePaths = new HashSet<>();
+        tree.walk(visit -> {
+            final String relativePath = visit.getRelativePath();
+            if (!relativePath.isEmpty()) {
+                relativePaths.add(relativePath);
+            }
+        });
+        return relativePaths;
+    }
+
     @Override
     public boolean containsReloadableResources() {
         return !pathTree.isArchiveOrigin();
@@ -229,7 +228,7 @@ private class Resource implements ClassPathResource {
         private volatile URL url;
 
         private Resource(PathVisit visit) {
-            name = visit.getRelativePath("/");
+            name = visit.getRelativePath();
             path = visit.getPath();
         }
 
@@ -271,14 +270,36 @@ public URL getUrl() {
                     }
                     return this.url = url;
                 }
-                return url = apply(tree -> tree.apply(name, visit -> visit == null ? null : visit.getUrl()));
+                return url = apply(this::getUrl);
             } catch (MalformedURLException e) {
                 throw new RuntimeException("Failed to translate " + path + " to URL", e);
             } finally {
                 lock.readLock().unlock();
             }
         }
 
+        /**
+         * URL for this resource in a given {@link OpenPathTree}.
+         * If the path tree contains the resource, its URL is returned.
+         * Otherwise, the method returns null.
+         *
+         * @param tree path tree to look for the resource in
+         * @return resource URL if it exists in a given path tree or null if it does not
+         */
+        private URL getUrl(OpenPathTree tree) {
+            return tree.apply(name, Resource::getUrl);
+        }
+
+        /**
+         * Returns a URL for a given {@link PathVisit} or null if the argument is null.
+         *
+         * @param visit visited path or null
+         * @return URL for a given path or null, in case the argument is null
+         */
+        private static URL getUrl(PathVisit visit) {
+            return visit == null ? null : visit.getUrl();
+        }
+
         @Override
         public byte[] getData() {
             lock.readLock().lock();
@@ -358,10 +379,18 @@ public boolean isDirectory() {
                 if (pathTree.isOpen()) {
                     return Files.isDirectory(path);
                 }
-                return apply(tree -> tree.apply(name, visit -> visit == null ? null : Files.isDirectory(visit.getPath())));
+                return apply(this::isDirectory);
             } finally {
                 lock.readLock().unlock();
             }
         }
+
+        private boolean isDirectory(OpenPathTree tree) {
+            return tree.apply(name, Resource::isDirectory);
+        }
+
+        private static boolean isDirectory(PathVisit visit) {
+            return visit != null && Files.isDirectory(visit.getPath());
+        }
     }
 }