Add the ability to resurrect closed classloaders

There have been a number of complaints about the new classloader closing
behavior. It is too aggressive about closing classloaders after test and
run. This commit softens the behavior by allowing a classloader to be
resurrected after close by creating a new zombie classloader that has
the same urls as the original classloader. After this commit, we always
close the classloaders when we are done, but they can still leak
file descriptors if a zombie is created.

To configure the behavior, I add the allowZombieClassLoaders key. If it
is false (which is default), we will warn but still allow them. If it
is true, then we silence the warning. In a later version of sbt, we can
change the semantics to be strict.

I verified after this change that I could add a shutdown hook in `run`
and it would be evaluated so long as I set `bgCopyClasspath := false`.
Otherwise the needed jars were deleted before the hooks could run.

Bonus: delete unused ResourceLoaderImpl class

build.sbt

@@ -672,12 +672,14 @@ lazy val mainProj = (project in file("main"))
       exclude[ReversedMissingMethodProblem]("sbt.internal.KeyIndex.*"),
       // internal
       exclude[IncompatibleMethTypeProblem]("sbt.internal.server.LanguageServerReporter.*"),
+
       // Changed signature or removed private[sbt] methods
       exclude[DirectMissingMethodProblem]("sbt.Classpaths.unmanagedLibs0"),
       exclude[DirectMissingMethodProblem]("sbt.Defaults.allTestGroupsTask"),
       exclude[DirectMissingMethodProblem]("sbt.Plugins.topologicalSort"),
       exclude[IncompatibleMethTypeProblem]("sbt.Defaults.allTestGroupsTask"),
-      exclude[DirectMissingMethodProblem]("sbt.StandardMain.shutdownHook")
+      exclude[DirectMissingMethodProblem]("sbt.StandardMain.shutdownHook"),
+      exclude[MissingClassProblem]("sbt.internal.ResourceLoaderImpl"),
     )
   )
   .configure(

main/src/main/java/sbt/internal/FlatLoader.java

@@ -7,17 +7,23 @@
 
 package sbt.internal;
 
-import java.io.IOException;
+import java.io.File;
 import java.net.URL;
-import java.net.URLClassLoader;
+import sbt.util.Logger;
+import scala.collection.Seq;
 
-final class FlatLoader extends URLClassLoader {
+final class FlatLoader extends LayeredClassLoaderImpl {
   static {
     ClassLoader.registerAsParallelCapable();
   }
 
-  FlatLoader(final URL[] urls, final ClassLoader parent) {
+  FlatLoader(
-    super(urls, parent);
+      final Seq<File> files,
+      final ClassLoader parent,
+      final File file,
+      final boolean allowZombies,
+      final Logger logger) {
+    super(files, parent, file, allowZombies, logger);
   }
 
   @Override
@@ -30,9 +36,4 @@ final class FlatLoader extends URLClassLoader {
     }
     return "FlatLoader(\n  parent = " + getParent() + "\n  jars = " + jars.toString() + ")";
   }
-
-  @Override
-  public void close() throws IOException {
-    if (SysProp.closeClassLoaders()) super.close();
-  }
 }

main/src/main/java/sbt/internal/LayeredClassLoader.java

@@ -8,14 +8,13 @@
 package sbt.internal;
 
 import java.io.File;
+import sbt.util.Logger;
 import scala.collection.Seq;
 
 final class LayeredClassLoader extends LayeredClassLoaderImpl {
-  LayeredClassLoader(
+  LayeredClassLoader(final Seq<File> classpath, final ClassLoader parent, final File tempDir, final
-      final Seq<File> classpath,
+      boolean allowZombies, final Logger logger) {
-      final ClassLoader parent,
+    super(classpath, parent, tempDir, allowZombies, logger);
-      final File tempDir) {
-    super(classpath, parent, tempDir);
   }
 
   static {

main/src/main/java/sbt/internal/ScalaLibraryClassLoader.java

@@ -7,7 +7,6 @@
 
 package sbt.internal;
 
-import java.io.IOException;
 import java.net.URL;
 import java.net.URLClassLoader;
 
@@ -32,9 +31,4 @@ final class ScalaLibraryClassLoader extends URLClassLoader {
     }
     return "ScalaLibraryClassLoader(" + builder + " parent = " + getParent() + ")";
   }
-
-  @Override
-  public void close() throws IOException {
-    if (SysProp.closeClassLoaders()) super.close();
-  }
 }

main/src/main/java/sbt/internal/ScalaReflectClassLoader.java

@@ -7,7 +7,6 @@
 
 package sbt.internal;
 
-import java.io.IOException;
 import java.net.URL;
 import java.net.URLClassLoader;
 
@@ -27,9 +26,4 @@ final class ScalaReflectClassLoader extends URLClassLoader {
   public String toString() {
     return "ScalaReflectClassLoader(" + jar + " parent = " + getParent() + ")";
   }
-
-  @Override
-  public void close() throws IOException {
-    if (SysProp.closeClassLoaders()) super.close();
-  }
 }

main/src/main/scala/sbt/Defaults.scala

@@ -32,7 +32,7 @@ import sbt.internal.CommandStrings.ExportStream
 import sbt.internal._
 import sbt.internal.classpath.AlternativeZincUtil
 import sbt.internal.inc.JavaInterfaceUtil._
-import sbt.internal.inc.classpath.ClasspathFilter
+import sbt.internal.inc.classpath.{ ClassLoaderCache, ClasspathFilter }
 import sbt.internal.inc.{ ZincLmUtil, ZincUtil }
 import sbt.internal.io.{ Source, WatchState }
 import sbt.internal.librarymanagement.mavenint.{
@@ -47,7 +47,6 @@ import sbt.internal.server.{
   LanguageServerReporter,
   ServerHandler
 }
-import sbt.nio.FileStamp.Formats.seqPathFileStampJsonFormatter
 import sbt.internal.testing.TestLogger
 import sbt.internal.util.Attributed.data
 import sbt.internal.util.Types._
@@ -70,6 +69,7 @@ import sbt.librarymanagement.CrossVersion.{ binarySbtVersion, binaryScalaVersion
 import sbt.librarymanagement._
 import sbt.librarymanagement.ivy._
 import sbt.librarymanagement.syntax._
+import sbt.nio.FileStamp.Formats.seqPathFileStampJsonFormatter
 import sbt.nio.Keys._
 import sbt.nio.file.syntax._
 import sbt.nio.file.{ FileTreeView, Glob, RecursiveGlob }
@@ -205,6 +205,7 @@ object Defaults extends BuildCommon {
       bgStop := bgStopTask.evaluated,
       bgWaitFor := bgWaitForTask.evaluated,
       bgCopyClasspath :== true,
+      allowZombieClassLoaders :== !SysProp.closeClassLoaders,
     )
 
   private[sbt] lazy val globalIvyCore: Seq[Setting[_]] =
@@ -813,7 +814,7 @@ object Defaults extends BuildCommon {
       allJars: Seq[File],
       libraryJars: Array[File],
       compilerJar: File,
-      classLoaderCache: sbt.internal.inc.classpath.ClassLoaderCache
+      classLoaderCache: ClassLoaderCache,
   ): ScalaInstance = {
     val allJarsDistinct = allJars.distinct
     val libraryLoader = classLoaderCache(libraryJars.toList)
@@ -837,7 +838,7 @@ object Defaults extends BuildCommon {
     val dummy = ScalaInstance(dir)(state.value.classLoaderCache.apply)
     Seq(dummy.loader, dummy.loaderLibraryOnly).foreach {
       case a: AutoCloseable => a.close()
-      case cl               =>
+      case _                =>
     }
     mkScalaInstance(
       dummy.version,

main/src/main/scala/sbt/Keys.scala

@@ -325,6 +325,7 @@ object Keys {
   val dependencyClasspathAsJars = taskKey[Classpath]("The classpath consisting of internal and external, managed and unmanaged dependencies, all as JARs.")
   val fullClasspathAsJars = taskKey[Classpath]("The exported classpath, consisting of build products and unmanaged and managed, internal and external dependencies, all as JARs.")
   val internalDependencyConfigurations = settingKey[Seq[(ProjectRef, Set[String])]]("The project configurations that this configuration depends on")
+  private[sbt] val allowZombieClassLoaders = settingKey[Boolean]("Allow a classloader that has previously been closed by `run` or `test` to continue loading classes.")
 
   val useCoursier = settingKey[Boolean]("Use Coursier for dependency resolution.").withRank(BSetting)
   val csrCacheDirectory = settingKey[File]("Coursier cache directory. Uses -Dsbt.coursier.home or Coursier's default.").withRank(CSetting)

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

@@ -22,6 +22,7 @@ import sbt.io.IO
 import sbt.nio.FileStamp
 import sbt.nio.FileStamp.LastModified
 import sbt.nio.Keys._
+import sbt.util.Logger
 
 private[sbt] object ClassLoaders {
   private[this] val interfaceLoader = classOf[sbt.testing.Framework].getClassLoader
@@ -38,6 +39,8 @@ private[sbt] object ClassLoaders {
       if (si.isManagedVersion) rawCP
       else si.libraryJars.map(j => j -> IO.getModifiedTimeOrZero(j)).toSeq ++ rawCP
     val exclude = dependencyJars(exportedProducts).value.toSet ++ si.libraryJars
+    val logger = state.value.globalLogging.full
+    val allowZombies = allowZombieClassLoaders.value
     buildLayers(
       strategy = classLoaderLayeringStrategy.value,
       si = si,
@@ -46,7 +49,9 @@ private[sbt] object ClassLoaders {
       cache = extendedClassLoaderCache.value,
       resources = ClasspathUtilities.createClasspathResources(fullCP.map(_._1), si),
       tmp = IO.createUniqueDirectory(taskTemporaryDirectory.value),
-      scope = resolvedScoped.value.scope
+      scope = resolvedScoped.value.scope,
+      logger = logger,
+      allowZombies = allowZombies,
     )
   }
 
@@ -77,6 +82,8 @@ private[sbt] object ClassLoaders {
         }
         val exclude = dependencyJars(exportedProducts).value.toSet ++ instance.libraryJars
         val allDeps = dependencyJars(dependencyClasspath).value.filterNot(exclude)
+        val logger = state.value.globalLogging.full
+        val allowZombies = allowZombieClassLoaders.value
         val newLoader =
           (classpath: Seq[File]) => {
             val mappings = classpath.map(f => f.getName -> f).toMap
@@ -89,7 +96,9 @@ private[sbt] object ClassLoaders {
               cache = extendedClassLoaderCache.value: @sbtUnchecked,
               resources = ClasspathUtilities.createClasspathResources(classpath, instance),
               tmp = taskTemporaryDirectory.value: @sbtUnchecked,
-              scope = resolvedScope
+              scope = resolvedScope,
+              logger = logger,
+              allowZombies = allowZombies,
             )
           }
         new Run(newLoader, trapExit.value)
@@ -121,11 +130,13 @@ private[sbt] object ClassLoaders {
       cache: ClassLoaderCache,
       resources: Map[String, String],
       tmp: File,
-      scope: Scope
+      scope: Scope,
+      logger: Logger,
+      allowZombies: Boolean
   ): ClassLoader = {
     val cpFiles = fullCP.map(_._1)
     strategy match {
-      case Flat => flatLoader(cpFiles, interfaceLoader)
+      case Flat => new FlatLoader(cpFiles, interfaceLoader, tmp, allowZombies, logger)
       case _ =>
         val layerDependencies = strategy match {
           case _: AllLibraryJars => true
@@ -161,7 +172,13 @@ private[sbt] object ClassLoaders {
             cache(
               allDependencies.toList.map(f => f -> IO.getModifiedTimeOrZero(f)),
               scalaReflectLayer,
-              () => new ReverseLookupClassLoaderHolder(allDependencies, scalaReflectLayer)
+              () =>
+                new ReverseLookupClassLoaderHolder(
+                  allDependencies,
+                  scalaReflectLayer,
+                  allowZombies,
+                  logger
+                )
             )
           } else scalaReflectLayer
 
@@ -177,7 +194,7 @@ private[sbt] object ClassLoaders {
           case cl =>
             cl.getParent match {
               case dl: ReverseLookupClassLoaderHolder => dl.checkout(cpFiles, tmp)
-              case _                                  => new LayeredClassLoader(dynamicClasspath, cl, tmp)
+              case _                                  => new LayeredClassLoader(dynamicClasspath, cl, tmp, allowZombies, logger)
             }
         }
     }
@@ -187,9 +204,6 @@ private[sbt] object ClassLoaders {
       key: sbt.TaskKey[Seq[Attributed[File]]]
   ): Def.Initialize[Task[Seq[File]]] = Def.task(data(key.value).filter(_.getName.endsWith(".jar")))
 
-  // helper methods
-  private def flatLoader(classpath: Seq[File], parent: ClassLoader): ClassLoader =
-    new FlatLoader(classpath.map(_.toURI.toURL).toArray, parent)
   private[this] def modifiedTimes(stamps: Seq[(Path, FileStamp)]): Seq[(File, Long)] = stamps.map {
     case (p, LastModified(lm)) => p.toFile -> lm
     case (p, _) =>

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

@@ -12,8 +12,8 @@ import java.net.{ URL, URLClassLoader }
 import java.util.concurrent.ConcurrentHashMap
 import java.util.concurrent.atomic.{ AtomicBoolean, AtomicReference }
 
-import sbt.internal.inc.classpath._
 import sbt.io.IO
+import sbt.util.Logger
 
 import scala.collection.JavaConverters._
 
@@ -27,11 +27,11 @@ import scala.collection.JavaConverters._
 private[internal] class LayeredClassLoaderImpl(
     classpath: Seq[File],
     parent: ClassLoader,
-    tempDir: File
+    tempDir: File,
-) extends URLClassLoader(classpath.map(_.toURI.toURL).toArray, parent)
+    allowZombies: Boolean,
-    with NativeLoader {
+    logger: Logger
+) extends ManagedClassLoader(classpath.toArray.map(_.toURI.toURL), parent, allowZombies, logger) {
   setTempDir(tempDir)
-  override def close(): Unit = if (SysProp.closeClassLoaders) super.close()
 }
 
 /**
@@ -58,10 +58,13 @@ private[internal] class LayeredClassLoaderImpl(
  */
 private[internal] final class ReverseLookupClassLoaderHolder(
     val classpath: Seq[File],
-    val parent: ClassLoader
+    val parent: ClassLoader,
+    val allowZombies: Boolean,
+    val logger: Logger
 ) extends URLClassLoader(Array.empty, null) {
   private[this] val cached: AtomicReference[ReverseLookupClassLoader] = new AtomicReference
   private[this] val closed = new AtomicBoolean(false)
+  private[this] val urls = classpath.map(_.toURI.toURL).toArray
 
   /**
    * Get a classloader. If there is a loader available in the cache, it will use that loader,
@@ -141,8 +144,9 @@ private[internal] final class ReverseLookupClassLoaderHolder(
    *
    */
   private class ReverseLookupClassLoader
-      extends URLClassLoader(classpath.map(_.toURI.toURL).toArray, parent)
+      extends ManagedClassLoader(urls, parent, allowZombies, logger)
       with NativeLoader {
+    override def getURLs: Array[URL] = urls
     private[this] val directDescendant: AtomicReference[BottomClassLoader] =
       new AtomicReference
     private[this] val dirty = new AtomicBoolean(false)
@@ -179,7 +183,6 @@ private[internal] final class ReverseLookupClassLoaderHolder(
     }
     override def loadClass(name: String, resolve: Boolean): Class[_] =
       loadClass(name, resolve, reverseLookup = true)
-    override def close(): Unit = if (SysProp.closeClassLoaders) super.close()
   }
 
   /**
@@ -202,7 +205,12 @@ private[internal] final class ReverseLookupClassLoaderHolder(
       dynamicClasspath: Seq[File],
       parent: ReverseLookupClassLoader,
       tempDir: File
-  ) extends URLClassLoader(dynamicClasspath.map(_.toURI.toURL).toArray, parent)
+  ) extends ManagedClassLoader(
+        dynamicClasspath.map(_.toURI.toURL).toArray,
+        parent,
+        allowZombies,
+        logger
+      )
       with NativeLoader {
     parent.setDescendant(this)
     setTempDir(tempDir)
@@ -236,7 +244,7 @@ private[internal] final class ReverseLookupClassLoaderHolder(
     }
     override def close(): Unit = {
       checkin(parent)
-      if (SysProp.closeClassLoaders) super.close()
+      super.close()
     }
   }
 }
@@ -294,21 +302,6 @@ private trait NativeLoader extends ClassLoader with AutoCloseable {
   }
 }
 
-private[internal] class ResourceLoaderImpl(
-    classpath: Seq[File],
-    parent: ClassLoader,
-    override val resources: Map[String, String]
-) extends URLClassLoader(classpath.map(_.toURI.toURL).toArray, parent)
-    with RawResources {
-  override def findClass(name: String): Class[_] = throw new ClassNotFoundException(name)
-  override def loadClass(name: String, resolve: Boolean): Class[_] = {
-    val clazz = parent.loadClass(name)
-    if (resolve) resolveClass(clazz)
-    clazz
-  }
-  override def toString: String = "ResourceLoader"
-}
-
 private[internal] object NativeLibs {
   private[this] val nativeLibs = new java.util.HashSet[File].asScala
   ShutdownHooks.add(() => {
@@ -327,3 +320,64 @@ private[internal] object NativeLibs {
     ()
   }
 }
+
+private sealed abstract class ManagedClassLoader(
+    urls: Array[URL],
+    parent: ClassLoader,
+    allowZombies: Boolean,
+    logger: Logger
+) extends URLClassLoader(urls, parent)
+    with NativeLoader {
+  private[this] val closed = new AtomicBoolean(false)
+  private[this] val printedWarning = new AtomicBoolean(false)
+  private[this] val zombieLoader = new AtomicReference[ZombieClassLoader]
+  private class ZombieClassLoader extends URLClassLoader(urls, this) {
+    def lookupClass(name: String): Class[_] =
+      try findClass(name)
+      catch {
+        case e: ClassNotFoundException =>
+          val deleted = urls.flatMap { u =>
+            val f = new File(u.getPath)
+            if (f.exists) None else Some(f)
+          }
+          if (deleted.toSeq.nonEmpty) {
+            // TODO - add doc link
+            val msg = s"Couldn't load class $name. " +
+              s"The following urls on the classpath do not exist:\n${deleted mkString "\n"}\n" +
+              "This may be due to shutdown hooks added during an invocation of `run`."
+            // logging may be shutdown at this point so we need to print directly to System.err.
+            System.err.println(msg)
+          }
+          throw e
+      }
+  }
+  private def getZombieLoader(name: String): ZombieClassLoader = {
+    if (printedWarning.compareAndSet(false, true) && !allowZombies) {
+      // TODO - Need to add link to documentation in website
+      val thread = Thread.currentThread
+      val msg =
+        s"$thread loading $name after test or run has completed. This is a likely resource leak."
+      logger.warn(msg)
+    }
+    zombieLoader.get match {
+      case null =>
+        val zb = new ZombieClassLoader
+        zombieLoader.set(zb)
+        zb
+      case zb => zb
+    }
+  }
+  override def findResource(name: String): URL = {
+    if (closed.get) getZombieLoader(name).findResource(name)
+    else super.findResource(name)
+  }
+  override def findClass(name: String): Class[_] = {
+    if (closed.get) getZombieLoader(name).lookupClass(name)
+    else super.findClass(name)
+  }
+  override def close(): Unit = {
+    closed.set(true)
+    Option(zombieLoader.getAndSet(null)).foreach(_.close())
+    super.close()
+  }
+}