luke
/
sbt
mirror of https://github.com/sbt/sbt.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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).
This commit is contained in:
Ethan Atkins 2019-12-06 08:46:22 -08:00
parent 4adb56ae9d
commit 8518c4b4fd
4 changed files with 80 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
main/src/main/java/sbt/internal/ReverseLookupClassLoader.java
View File

@ -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

34
main/src/main/java/sbt/internal/ScalaTestFrameworkClassLoader.java Normal file
View File

@ -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() + ")";
}
}

54
main/src/main/scala/sbt/internal/ClassLoaders.scala
View File

@ -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)
}

6
main/src/main/scala/sbt/internal/LayeredClassLoaders.scala
View File

@ -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,