Merge pull request #5273 from eatkins/scalatest-framework-loader

Place scalatest framework jar in its own classloader

main/src/main/java/sbt/internal/ReverseLookupClassLoader.java

@@ -8,9 +8,7 @@
 package sbt.internal;
 
 import java.io.File;
-import java.io.IOException;
 import java.net.URL;
-import java.net.URLClassLoader;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import sbt.util.Logger;
@@ -46,7 +44,6 @@ final class ReverseLookupClassLoader extends ManagedClassLoader {
   private final AtomicReference<BottomClassLoader> directDescendant = new AtomicReference<>();
   private final AtomicBoolean dirty = new AtomicBoolean(false);
   private final ClassLoadingLock classLoadingLock = new ClassLoadingLock();
-  private final AtomicReference<ResourceLoader> resourceLoader = new AtomicReference<>();
   private final ClassLoader parent;
 
   boolean isDirty() {
@@ -57,26 +54,14 @@ final class ReverseLookupClassLoader extends ManagedClassLoader {
     directDescendant.set(bottomClassLoader);
   }
 
-  private class ResourceLoader extends URLClassLoader {
-    ResourceLoader(final URL[] urls) {
-      super(urls, parent);
-    }
-
-    final URL lookup(final String name) {
-      return findResource(name);
-    }
-  }
-
   @Override
   public URL findResource(String name) {
-    final ResourceLoader loader = resourceLoader.get();
-    return loader == null ? null : loader.lookup(name);
+    final URL url = super.findResource(name);
+    return url != null ? url : directDescendant.get().findResource(name);
   }
 
-  void setup(final File tmpDir, final URL[] urls) throws IOException {
+  void setup(final File tmpDir) {
     setTempDir(tmpDir);
-    final ResourceLoader previous = resourceLoader.getAndSet(new ResourceLoader(urls));
-    if (previous != null) previous.close();
   }
 
   @Override

main/src/main/java/sbt/internal/ScalaTestFrameworkClassLoader.java

@@ -0,0 +1,34 @@
+/*
+ * sbt
+ * Copyright 2011 - 2018, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt.internal;
+
+    import java.net.URL;
+    import java.net.URLClassLoader;
+
+final class ScalaTestFrameworkClassLoader extends URLClassLoader {
+  static {
+    ClassLoader.registerAsParallelCapable();
+  }
+
+  private final URL[] jars;
+
+  ScalaTestFrameworkClassLoader(final URL[] jars, final ClassLoader parent) {
+    super(jars, parent);
+    this.jars = jars;
+  }
+
+  @Override
+  public String toString() {
+    final StringBuilder builder = new StringBuilder();
+    for (int i = 0; i < jars.length; ++ i) {
+      builder.append(jars[i].toString());
+      if (i < jars.length - 2) builder.append(", ");
+    }
+    return "ScalaTestFrameworkClassLoader(" + builder + " parent = " + getParent() + ")";
+  }
+}

main/src/main/scala/sbt/internal/ClassLoaders.scala

@@ -156,10 +156,25 @@ private[sbt] object ClassLoaders {
         val cpFiles = fullCP.map(_._1)
 
         val allDependencies = cpFiles.filter(allDependenciesSet)
-        val scalaReflectJar = allDependencies.collectFirst {
-          case f if f.getName == "scala-reflect.jar" =>
-            si.allJars.find(_.getName == "scala-reflect.jar")
-        }.flatten
+        /*
+         * We need to put scalatest and its dependencies (scala-xml) in its own layer so that the
+         * classloader for scalatest is generally not ever closed. This is because one project
+         * closing its scalatest loader can cause a crash in another project if that project
+         * is reading the scalatest bundle properties because the underlying JarFile instance is
+         * shared across URLClassLoaders if it points to the same jar.
+         *
+         * TODO: Make this configurable and/or add other frameworks in this layer.
+         */
+        val (frameworkDependencies, otherDependencies) = allDependencies.partition { d =>
+          val name = d.getName
+          name.startsWith("scalatest_") || name.startsWith("scalactic_") ||
+          name.startsWith("scala-xml_")
+        }
+        def isReflectJar(f: File) =
+          f.getName == "scala-reflect.jar" || f.getName.startsWith("scala-reflect-")
+        val scalaReflectJar = otherDependencies.collectFirst {
+          case f if isReflectJar(f) => si.allJars.find(isReflectJar).getOrElse(f)
+        }
         val scalaReflectLayer = scalaReflectJar
           .map { file =>
             cache.apply(
@@ -170,34 +185,45 @@ private[sbt] object ClassLoaders {
           }
           .getOrElse(scalaLibraryLayer)
 
-        // layer 2 (optional if in the test config and the runtime layer is not shared)
+        // layer 2 framework jars
+        val frameworkLayer =
+          if (frameworkDependencies.isEmpty) scalaReflectLayer
+          else {
+            cache.apply(
+              frameworkDependencies.map(file => file -> IO.getModifiedTimeOrZero(file)).toList,
+              scalaLibraryLayer,
+              () => new ScalaTestFrameworkClassLoader(frameworkDependencies.urls, scalaReflectLayer)
+            )
+          }
+
+        // layer 3 (optional if in the test config and the runtime layer is not shared)
         val dependencyLayer: ClassLoader =
-          if (layerDependencies && allDependencies.nonEmpty) {
+          if (layerDependencies && otherDependencies.nonEmpty) {
             cache(
-              allDependencies.toList.map(f => f -> IO.getModifiedTimeOrZero(f)),
-              scalaReflectLayer,
+              otherDependencies.toList.map(f => f -> IO.getModifiedTimeOrZero(f)),
+              frameworkLayer,
               () =>
                 new ReverseLookupClassLoaderHolder(
-                  allDependencies,
-                  scalaReflectLayer,
+                  otherDependencies,
+                  frameworkLayer,
                   allowZombies,
                   logger
                 )
             )
-          } else scalaReflectLayer
+          } else frameworkLayer
 
         val scalaJarNames = (si.libraryJars ++ scalaReflectJar).map(_.getName).toSet
         // layer 3
         val filteredSet =
           if (layerDependencies) allDependencies.toSet ++ si.libraryJars ++ scalaReflectJar
-          else Set(si.libraryJars ++ scalaReflectJar: _*)
+          else Set(frameworkDependencies ++ si.libraryJars ++ scalaReflectJar: _*)
         val dynamicClasspath = cpFiles.filterNot(f => filteredSet(f) || scalaJarNames(f.getName))
         dependencyLayer match {
           case dl: ReverseLookupClassLoaderHolder =>
-            dl.checkout(cpFiles, tmp)
+            dl.checkout(dynamicClasspath, tmp)
           case cl =>
             cl.getParent match {
-              case dl: ReverseLookupClassLoaderHolder => dl.checkout(cpFiles, tmp)
+              case dl: ReverseLookupClassLoaderHolder => dl.checkout(dynamicClasspath, tmp)
               case _ =>
                 new LayeredClassLoader(dynamicClasspath.urls, cl, tmp, allowZombies, logger)
             }

main/src/main/scala/sbt/internal/LayeredClassLoaders.scala

@@ -55,7 +55,7 @@ private[internal] final class ReverseLookupClassLoaderHolder(
    *
    * @return a ClassLoader
    */
-  def checkout(fullClasspath: Seq[File], tempDir: File): ClassLoader = {
+  def checkout(dynamicClasspath: Seq[File], tempDir: File): ClassLoader = {
     if (closed.get()) {
       val msg = "Tried to extract class loader from closed ReverseLookupClassLoaderHolder. " +
         "Try running the `clearCaches` command and re-trying."
@@ -65,10 +65,10 @@ private[internal] final class ReverseLookupClassLoaderHolder(
       case null => new ReverseLookupClassLoader(urls, parent, allowZombies, logger)
       case c    => c
     }
-    reverseLookupClassLoader.setup(tempDir, fullClasspath.map(_.toURI.toURL).toArray)
+    reverseLookupClassLoader.setup(tempDir)
     new BottomClassLoader(
       ReverseLookupClassLoaderHolder.this,
-      fullClasspath.map(_.toURI.toURL).toArray,
+      dynamicClasspath.map(_.toURI.toURL).toArray,
       reverseLookupClassLoader,
       tempDir,
       allowZombies,