From 46eaa564d59d48dd0fc87c694209f7e3a06043fa Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sun, 15 Sep 2019 17:57:29 -0700 Subject: [PATCH 1/3] Add 1.3.0 to mima --- build.sbt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.sbt b/build.sbt index 98da9bcee..b2f880fd2 100644 --- a/build.sbt +++ b/build.sbt @@ -121,7 +121,7 @@ def sbt10Plus = "1.2.7", "1.2.8", ) ++ sbt13Plus -def sbt13Plus = Seq() // Add sbt 1.3+ stable versions when released +def sbt13Plus = Seq("1.3.0") def mimaSettings = mimaSettingsSince(sbt10Plus) def mimaSettingsSince(versions: Seq[String]) = Def settings ( From 231d7966d0f303b75e2c7d295af3a59523a7e3db Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Fri, 13 Sep 2019 10:49:09 -0700 Subject: [PATCH 2/3] 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 | 4 +- .../main/java/sbt/internal/FlatLoader.java | 21 ++-- .../java/sbt/internal/LayeredClassLoader.java | 9 +- .../sbt/internal/ScalaLibraryClassLoader.java | 6 - .../sbt/internal/ScalaReflectClassLoader.java | 6 - main/src/main/scala/sbt/Defaults.scala | 9 +- main/src/main/scala/sbt/Keys.scala | 1 + .../scala/sbt/internal/ClassLoaders.scala | 32 ++++-- .../sbt/internal/LayeredClassLoaders.scala | 104 +++++++++++++----- 9 files changed, 126 insertions(+), 66 deletions(-) diff --git a/build.sbt b/build.sbt index b2f880fd2..851694fe4 100644 --- a/build.sbt +++ b/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( diff --git a/main/src/main/java/sbt/internal/FlatLoader.java b/main/src/main/java/sbt/internal/FlatLoader.java index 1ded2a242..83fcabe02 100644 --- a/main/src/main/java/sbt/internal/FlatLoader.java +++ b/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) { - super(urls, parent); + FlatLoader( + final Seq 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(); - } } diff --git a/main/src/main/java/sbt/internal/LayeredClassLoader.java b/main/src/main/java/sbt/internal/LayeredClassLoader.java index a66f1d488..132e7f7cc 100644 --- a/main/src/main/java/sbt/internal/LayeredClassLoader.java +++ b/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( - final Seq classpath, - final ClassLoader parent, - final File tempDir) { - super(classpath, parent, tempDir); + LayeredClassLoader(final Seq classpath, final ClassLoader parent, final File tempDir, final + boolean allowZombies, final Logger logger) { + super(classpath, parent, tempDir, allowZombies, logger); } static { diff --git a/main/src/main/java/sbt/internal/ScalaLibraryClassLoader.java b/main/src/main/java/sbt/internal/ScalaLibraryClassLoader.java index 2baff55bc..7d9d4f679 100644 --- a/main/src/main/java/sbt/internal/ScalaLibraryClassLoader.java +++ b/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(); - } } diff --git a/main/src/main/java/sbt/internal/ScalaReflectClassLoader.java b/main/src/main/java/sbt/internal/ScalaReflectClassLoader.java index 18461547b..b8a3fff17 100644 --- a/main/src/main/java/sbt/internal/ScalaReflectClassLoader.java +++ b/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(); - } } diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 072a7595d..4487fb2a7 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/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, diff --git a/main/src/main/scala/sbt/Keys.scala b/main/src/main/scala/sbt/Keys.scala index a54df46c6..76df1e995 100644 --- a/main/src/main/scala/sbt/Keys.scala +++ b/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) diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index 6ecc8b10e..1387d4abb 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/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, _) => diff --git a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala b/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala index 869c5e39c..5f0da410f 100644 --- a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala +++ b/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 -) extends URLClassLoader(classpath.map(_.toURI.toURL).toArray, parent) - with NativeLoader { + tempDir: File, + allowZombies: Boolean, + 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() + } +} From c2dc22f7dccff83c2815a1f52b40e2366b3b9595 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Wed, 18 Sep 2019 19:27:26 -0700 Subject: [PATCH 3/3] Make allowZombieClassLoaders public For forward binary compatibility in the 1.3.x series, this key needed to be private[sbt], but we can make it public in 1.4.x. --- main/src/main/scala/sbt/Keys.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main/src/main/scala/sbt/Keys.scala b/main/src/main/scala/sbt/Keys.scala index 76df1e995..ca5bc5257 100644 --- a/main/src/main/scala/sbt/Keys.scala +++ b/main/src/main/scala/sbt/Keys.scala @@ -325,7 +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 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)