From a177c386c08d828b5a363eb5ac592e3cb8d43e6f Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Thu, 12 Dec 2019 16:52:25 -0800 Subject: [PATCH] Add closeClassLoader setting There have been a number of issues that have come up because of sbt 1.3.0 aggressively closing classloaders. While these issues have been quite useful in helping us determine some issues related to classloader lifecycle, we should give users the option to prevent sbt from closing the classloaders. I also noticed that the classloader-cache/spark test has been occasionally segfaulting on travis so I disable classloader closing in that test. --- main/src/main/java/sbt/internal/BottomClassLoader.java | 3 ++- main/src/main/java/sbt/internal/FlatLoader.java | 3 ++- .../src/main/java/sbt/internal/LayeredClassLoader.java | 4 ++-- .../src/main/java/sbt/internal/ManagedClassLoader.java | 8 +++++--- .../java/sbt/internal/ReverseLookupClassLoader.java | 8 ++++++-- main/src/main/scala/sbt/Defaults.scala | 3 ++- main/src/main/scala/sbt/Keys.scala | 1 + main/src/main/scala/sbt/internal/ClassLoaders.scala | 10 ++++++++-- .../main/scala/sbt/internal/LayeredClassLoaders.scala | 4 +++- sbt/src/sbt-test/classloader-cache/spark/build.sbt | 2 ++ 10 files changed, 33 insertions(+), 13 deletions(-) diff --git a/main/src/main/java/sbt/internal/BottomClassLoader.java b/main/src/main/java/sbt/internal/BottomClassLoader.java index 4607777ee..58997c4db 100644 --- a/main/src/main/java/sbt/internal/BottomClassLoader.java +++ b/main/src/main/java/sbt/internal/BottomClassLoader.java @@ -32,9 +32,10 @@ final class BottomClassLoader extends ManagedClassLoader { final URL[] dynamicClasspath, final ReverseLookupClassLoader reverseLookupClassLoader, final File tempDir, + final boolean close, final boolean allowZombies, final Logger logger) { - super(dynamicClasspath, reverseLookupClassLoader, allowZombies, logger); + super(dynamicClasspath, reverseLookupClassLoader, close, allowZombies, logger); setTempDir(tempDir); this.holder = holder; this.parent = reverseLookupClassLoader; diff --git a/main/src/main/java/sbt/internal/FlatLoader.java b/main/src/main/java/sbt/internal/FlatLoader.java index 519610768..203635412 100644 --- a/main/src/main/java/sbt/internal/FlatLoader.java +++ b/main/src/main/java/sbt/internal/FlatLoader.java @@ -20,9 +20,10 @@ final class FlatLoader extends ManagedClassLoader { final URL[] urls, final ClassLoader parent, final File file, + final boolean close, final boolean allowZombies, final Logger logger) { - super(urls, parent, allowZombies, logger); + super(urls, parent, close, allowZombies, logger); setTempDir(file); } diff --git a/main/src/main/java/sbt/internal/LayeredClassLoader.java b/main/src/main/java/sbt/internal/LayeredClassLoader.java index 088af46d6..32641e828 100644 --- a/main/src/main/java/sbt/internal/LayeredClassLoader.java +++ b/main/src/main/java/sbt/internal/LayeredClassLoader.java @@ -12,9 +12,9 @@ import java.net.URL; import sbt.util.Logger; final class LayeredClassLoader extends ManagedClassLoader { - LayeredClassLoader(final URL[] classpath, final ClassLoader parent, final File tempDir, final + LayeredClassLoader(final URL[] classpath, final ClassLoader parent, final File tempDir, final boolean close, final boolean allowZombies, final Logger logger) { - super(classpath, parent, allowZombies, logger); + super(classpath, parent, close, allowZombies, logger); setTempDir(tempDir); } diff --git a/main/src/main/java/sbt/internal/ManagedClassLoader.java b/main/src/main/java/sbt/internal/ManagedClassLoader.java index a1701a3cb..59dac40a4 100644 --- a/main/src/main/java/sbt/internal/ManagedClassLoader.java +++ b/main/src/main/java/sbt/internal/ManagedClassLoader.java @@ -20,6 +20,7 @@ abstract class ManagedClassLoader extends URLClassLoader implements NativeLoader private final AtomicBoolean closed = new AtomicBoolean(false); private final AtomicBoolean printedWarning = new AtomicBoolean(false); private final AtomicReference zombieLoader = new AtomicReference<>(); + private final boolean close; private final boolean allowZombies; private final Logger logger; private final NativeLookup nativeLookup = new NativeLookup(); @@ -29,8 +30,9 @@ abstract class ManagedClassLoader extends URLClassLoader implements NativeLoader } ManagedClassLoader( - final URL[] urls, final ClassLoader parent, final boolean allowZombies, final Logger logger) { + final URL[] urls, final ClassLoader parent, final boolean close, final boolean allowZombies, final Logger logger) { super(urls, parent); + this.close = close; this.allowZombies = allowZombies; this.logger = logger; } @@ -103,8 +105,8 @@ abstract class ManagedClassLoader extends URLClassLoader implements NativeLoader @Override public void close() throws IOException { final ZombieClassLoader zb = zombieLoader.getAndSet(null); - if (zb != null) zb.close(); - if (closed.compareAndSet(false, true)) super.close(); + if (zb != null && close) zb.close(); + if (close && closed.compareAndSet(false, true)) super.close(); } @Override diff --git a/main/src/main/java/sbt/internal/ReverseLookupClassLoader.java b/main/src/main/java/sbt/internal/ReverseLookupClassLoader.java index 2bce9f755..69ed365a2 100644 --- a/main/src/main/java/sbt/internal/ReverseLookupClassLoader.java +++ b/main/src/main/java/sbt/internal/ReverseLookupClassLoader.java @@ -36,8 +36,12 @@ import sbt.util.Logger; */ final class ReverseLookupClassLoader extends ManagedClassLoader { ReverseLookupClassLoader( - final URL[] urls, final ClassLoader parent, final boolean allowZombies, final Logger logger) { - super(urls, parent, allowZombies, logger); + final URL[] urls, + final ClassLoader parent, + final boolean close, + final boolean allowZombies, + final Logger logger) { + super(urls, parent, close, allowZombies, logger); this.parent = parent; } diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 42e9eeecf..3cfbcd75c 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -205,7 +205,8 @@ object Defaults extends BuildCommon { bgStop := bgStopTask.evaluated, bgWaitFor := bgWaitForTask.evaluated, bgCopyClasspath :== true, - allowZombieClassLoaders :== !SysProp.closeClassLoaders, + closeClassLoaders :== SysProp.closeClassLoaders, + allowZombieClassLoaders :== true, ) private[sbt] lazy val globalIvyCore: Seq[Setting[_]] = diff --git a/main/src/main/scala/sbt/Keys.scala b/main/src/main/scala/sbt/Keys.scala index fa68eff42..d18fbb565 100644 --- a/main/src/main/scala/sbt/Keys.scala +++ b/main/src/main/scala/sbt/Keys.scala @@ -327,6 +327,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") + val closeClassLoaders = settingKey[Boolean]("Close classloaders in run and test when the task completes.").withRank(DSetting) 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) diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index 930124037..ff343c50b 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaders.scala @@ -44,6 +44,7 @@ private[sbt] object ClassLoaders { 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 close = closeClassLoaders.value val allowZombies = allowZombieClassLoaders.value buildLayers( strategy = classLoaderLayeringStrategy.value, @@ -55,6 +56,7 @@ private[sbt] object ClassLoaders { tmp = IO.createUniqueDirectory(taskTemporaryDirectory.value), scope = resolvedScoped.value.scope, logger = logger, + close = close, allowZombies = allowZombies, ) } @@ -88,6 +90,7 @@ private[sbt] object ClassLoaders { val allDeps = dependencyJars(dependencyClasspath).value.filterNot(exclude) val logger = state.value.globalLogging.full val allowZombies = allowZombieClassLoaders.value + val close = closeClassLoaders.value val newLoader = (classpath: Seq[File]) => { val mappings = classpath.map(f => f.getName -> f).toMap @@ -102,6 +105,7 @@ private[sbt] object ClassLoaders { tmp = taskTemporaryDirectory.value: @sbtUnchecked, scope = resolvedScope, logger = logger, + close = close, allowZombies = allowZombies, ) } @@ -136,11 +140,12 @@ private[sbt] object ClassLoaders { tmp: File, scope: Scope, logger: Logger, + close: Boolean, allowZombies: Boolean ): ClassLoader = { val cpFiles = fullCP.map(_._1) strategy match { - case Flat => new FlatLoader(cpFiles.urls, interfaceLoader, tmp, allowZombies, logger) + case Flat => new FlatLoader(cpFiles.urls, interfaceLoader, tmp, close, allowZombies, logger) case _ => val layerDependencies = strategy match { case _: AllLibraryJars => true @@ -206,6 +211,7 @@ private[sbt] object ClassLoaders { new ReverseLookupClassLoaderHolder( otherDependencies, frameworkLayer, + close, allowZombies, logger ) @@ -225,7 +231,7 @@ private[sbt] object ClassLoaders { cl.getParent match { case dl: ReverseLookupClassLoaderHolder => dl.checkout(dynamicClasspath, tmp) case _ => - new LayeredClassLoader(dynamicClasspath.urls, cl, tmp, allowZombies, logger) + new LayeredClassLoader(dynamicClasspath.urls, cl, tmp, close, allowZombies, logger) } } } diff --git a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala b/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala index ae56b2723..5b55bde01 100644 --- a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala @@ -42,6 +42,7 @@ import scala.collection.JavaConverters._ private[internal] final class ReverseLookupClassLoaderHolder( val classpath: Seq[File], val parent: ClassLoader, + val closeThis: Boolean, val allowZombies: Boolean, val logger: Logger ) extends URLClassLoader(Array.empty, null) { @@ -62,7 +63,7 @@ private[internal] final class ReverseLookupClassLoaderHolder( throw new IllegalStateException(msg) } val reverseLookupClassLoader = cached.getAndSet(null) match { - case null => new ReverseLookupClassLoader(urls, parent, allowZombies, logger) + case null => new ReverseLookupClassLoader(urls, parent, closeThis, allowZombies, logger) case c => c } reverseLookupClassLoader.setup(tempDir) @@ -71,6 +72,7 @@ private[internal] final class ReverseLookupClassLoaderHolder( dynamicClasspath.map(_.toURI.toURL).toArray, reverseLookupClassLoader, tempDir, + closeThis, allowZombies, logger ) diff --git a/sbt/src/sbt-test/classloader-cache/spark/build.sbt b/sbt/src/sbt-test/classloader-cache/spark/build.sbt index d64005ef4..85ebe2134 100644 --- a/sbt/src/sbt-test/classloader-cache/spark/build.sbt +++ b/sbt/src/sbt-test/classloader-cache/spark/build.sbt @@ -5,3 +5,5 @@ version := "1.0" scalaVersion := "2.12.10" libraryDependencies += "org.apache.spark" %% "spark-sql" % "2.4.3" + +Compile / closeClassLoaders := false