From 8518c4b4fd5304d9a82f365c0c62f1352aad2179 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Fri, 6 Dec 2019 08:46:22 -0800 Subject: [PATCH] Place scalatest framework jar in its own classloader Closing the ManagedClassLoader generated by test can cause nonlocal effects because the jdk shares some JarFile resources across multiple URLClassLoaders. As a result, if one classloader is trying to load a resource and the classloader is closed, it might cause the resource loading to fail (see https://github.com/sbt/sbt/issues/5262). This can be fixed by moving the scalatest framework jar (and its dependencies) into an additional classloader layer that sits between the scala library loader and the rest of the test dependencies. In addition to adding the new layer, I reworked the ReverseLookupClassLoader to use its dependent classloader to find resources that may below it in the classloading hierarchy rather than constructing an entirely new classloader for resources. After this change, I was able to run test in the repro project: https://github.com/rjmac/sbt-5262 1000 times with no failures. Note that the repro is sensitive to the jdk used. I could not reproduce with jdk11 but I could typically induce a failure within 20 or so runs with jdk8. I benchmarked this change with https://github.com/eatkins/scala-build-watch-performance and performance was roughly the same as 1.3.4 with turbo mode and about 200-250ms faster in non-turbo mode (which can be explained by the time to load the scalatest classes). --- .../internal/ReverseLookupClassLoader.java | 21 ++------ .../ScalaTestFrameworkClassLoader.java | 34 ++++++++++++ .../scala/sbt/internal/ClassLoaders.scala | 54 ++++++++++++++----- .../sbt/internal/LayeredClassLoaders.scala | 6 +-- 4 files changed, 80 insertions(+), 35 deletions(-) create mode 100644 main/src/main/java/sbt/internal/ScalaTestFrameworkClassLoader.java diff --git a/main/src/main/java/sbt/internal/ReverseLookupClassLoader.java b/main/src/main/java/sbt/internal/ReverseLookupClassLoader.java index 5a155005c..2bce9f755 100644 --- a/main/src/main/java/sbt/internal/ReverseLookupClassLoader.java +++ b/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 directDescendant = new AtomicReference<>(); private final AtomicBoolean dirty = new AtomicBoolean(false); private final ClassLoadingLock classLoadingLock = new ClassLoadingLock(); - private final AtomicReference 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 diff --git a/main/src/main/java/sbt/internal/ScalaTestFrameworkClassLoader.java b/main/src/main/java/sbt/internal/ScalaTestFrameworkClassLoader.java new file mode 100644 index 000000000..33d420bce --- /dev/null +++ b/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() + ")"; + } +} diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index bdf25ebe6..930124037 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/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) } diff --git a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala b/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala index 3f50f8c2a..ae56b2723 100644 --- a/main/src/main/scala/sbt/internal/LayeredClassLoaders.scala +++ b/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,