From 13cdbb5ea681985a09f812826aeebd0099127f82 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 1 Apr 2019 14:22:33 -0700 Subject: [PATCH 1/7] Don't make redundant ClassLoaderCache instance I noticed that sometimes multiple ClassLoaderCache instances were created in each configuraiton. I believe this was due to the use of inConfig(...)(...) causing multiple caches to be created. Long term, I'm not sure that taskRepository and classLoaderCache are the right solutions so I made classLoaderCache private[sbt] as well. --- main/src/main/scala/sbt/Defaults.scala | 4 ++-- main/src/main/scala/sbt/Keys.scala | 2 +- .../main/scala/sbt/internal/ClassLoaderCache.scala | 13 ++++--------- main/src/main/scala/sbt/internal/ClassLoaders.scala | 6 +++--- 4 files changed, 10 insertions(+), 15 deletions(-) diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 2b69c710a..9fc852485 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -1772,7 +1772,7 @@ object Defaults extends BuildCommon { Classpaths.addUnmanagedLibrary ++ Vector( TaskRepository.proxy( - classLoaderCache, + Compile / classLoaderCache, // We need a cache of size four so that the subset of the runtime dependencies that are used // by the test task layers may be cached without evicting the runtime classloader layers. The // cache size should be a multiple of two to support snapshot layers. @@ -1783,7 +1783,7 @@ object Defaults extends BuildCommon { lazy val testSettings: Seq[Setting[_]] = configSettings ++ testTasks ++ Vector( TaskRepository.proxy( - classLoaderCache, + Test / classLoaderCache, // We need a cache of size two for the test dependency layers (regular and snapshot). ClassLoaderCache(2) ) diff --git a/main/src/main/scala/sbt/Keys.scala b/main/src/main/scala/sbt/Keys.scala index a056afca2..0f5894bbf 100644 --- a/main/src/main/scala/sbt/Keys.scala +++ b/main/src/main/scala/sbt/Keys.scala @@ -503,7 +503,7 @@ object Keys { val resolvedScoped = Def.resolvedScoped val pluginData = taskKey[PluginData]("Information from the plugin build needed in the main build definition.").withRank(DTask) val globalPluginUpdate = taskKey[UpdateReport]("A hook to get the UpdateReport of the global plugin.").withRank(DTask) - val classLoaderCache = taskKey[internal.ClassLoaderCache]("The cache of ClassLoaders to be used for layering in tasks that invoke other java code").withRank(DTask) + private[sbt] val classLoaderCache = taskKey[internal.ClassLoaderCache]("The cache of ClassLoaders to be used for layering in tasks that invoke other java code").withRank(DTask) private[sbt] val taskRepository = AttributeKey[TaskRepository.Repr]("task-repository", "A repository that can be used to cache arbitrary values for a given task key that can be read or filled during task evaluation.", 10000) private[sbt] val taskCancelStrategy = settingKey[State => TaskCancellationStrategy]("Experimental task cancellation handler.").withRank(DTask) diff --git a/main/src/main/scala/sbt/internal/ClassLoaderCache.scala b/main/src/main/scala/sbt/internal/ClassLoaderCache.scala index 3a2c3db04..90f4ea53b 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaderCache.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaderCache.scala @@ -14,17 +14,17 @@ import sbt.internal.util.TypeFunctions.Id import scala.annotation.tailrec -sealed trait ClassLoaderCache +private[sbt] sealed trait ClassLoaderCache extends Repository[Id, (Seq[File], ClassLoader, Map[String, String], File), ClassLoader] -object ClassLoaderCache { +private[sbt] object ClassLoaderCache { private type Resources = Map[String, String] private sealed trait CachedClassLoader extends ClassLoader { def close(): Unit } private sealed trait StableClassLoader extends CachedClassLoader private sealed trait SnapshotClassLoader extends CachedClassLoader - def apply(maxSize: Int): ClassLoaderCache = + def apply(maxSize: Int): ClassLoaderCache = { new ClassLoaderCache { private final def mktmp(tmp: File): File = if (maxSize > 0) Files.createTempDirectory("sbt-jni").toFile else tmp @@ -89,10 +89,5 @@ object ClassLoaderCache { } } } - def empty(newLoader: (Seq[File], ClassLoader, Resources, File) => ClassLoader): ClassLoaderCache = - new ClassLoaderCache { - override def get(key: (Seq[File], ClassLoader, Resources, File)): ClassLoader = - newLoader.tupled(key) - override def close(): Unit = {} - } + } } diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index 79a303f80..865ee4be8 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaders.scala @@ -18,7 +18,7 @@ import sbt.internal.inc.classpath.{ ClasspathUtilities, DualLoader, NullLoader } import sbt.internal.util.Attributed import sbt.internal.util.Attributed.data import sbt.io.IO -import sbt.librarymanagement.Configurations.Runtime +import sbt.librarymanagement.Configurations.{ Runtime, Test } import PrettyPrint.indent import scala.annotation.tailrec @@ -55,7 +55,7 @@ private[sbt] object ClassLoaders { dependencyJars(dependencyClasspath).value.filterNot(exclude).toSet, interfaceLoader, (Runtime / classLoaderCache).value, - classLoaderCache.value, + (Test / classLoaderCache).value, ClasspathUtilities.createClasspathResources(fullCP, si), IO.createUniqueDirectory(taskTemporaryDirectory.value), resolvedScoped.value.scope @@ -88,7 +88,7 @@ private[sbt] object ClassLoaders { s.log.warn(s"$showJavaOptions will be ignored, $showFork is set to false") } val runtimeCache = (Runtime / classLoaderCache).value - val testCache = classLoaderCache.value + val testCache = (Test / classLoaderCache).value val exclude = dependencyJars(exportedProducts).value.toSet ++ instance.allJars val newLoader = (classpath: Seq[File]) => { From cb7fbfc81036b7b3de65f177c6ebd7082c5c2814 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 1 Apr 2019 13:36:53 -0700 Subject: [PATCH 2/7] Use named parameters --- .../scala/sbt/internal/ClassLoaders.scala | 49 ++++++++++--------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index 865ee4be8..6b3aed0ba 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaders.scala @@ -48,17 +48,18 @@ private[sbt] object ClassLoaders { val fullCP = if (si.isManagedVersion) rawCP else si.allJars.toSeq ++ rawCP val exclude = dependencyJars(exportedProducts).value.toSet ++ si.allJars.toSeq buildLayers( - classLoaderLayeringStrategy.value, - si, - fullCP, - dependencyJars(Runtime / dependencyClasspath).value.filterNot(exclude), - dependencyJars(dependencyClasspath).value.filterNot(exclude).toSet, - interfaceLoader, - (Runtime / classLoaderCache).value, - (Test / classLoaderCache).value, - ClasspathUtilities.createClasspathResources(fullCP, si), - IO.createUniqueDirectory(taskTemporaryDirectory.value), - resolvedScoped.value.scope + strategy = classLoaderLayeringStrategy.value, + si = si, + fullCP = fullCP, + rawRuntimeDependencies = + dependencyJars(Runtime / dependencyClasspath).value.filterNot(exclude), + allDependencies = dependencyJars(dependencyClasspath).value.filterNot(exclude).toSet, + base = interfaceLoader, + runtimeCache = (Runtime / classLoaderCache).value, + testCache = (Test / classLoaderCache).value, + resources = ClasspathUtilities.createClasspathResources(fullCP, si), + tmp = IO.createUniqueDirectory(taskTemporaryDirectory.value), + scope = resolvedScoped.value.scope ) } @@ -93,18 +94,20 @@ private[sbt] object ClassLoaders { val newLoader = (classpath: Seq[File]) => { buildLayers( - classLoaderLayeringStrategy.value: @sbtUnchecked, - instance, - classpath, - (dependencyJars(Runtime / dependencyClasspath).value: @sbtUnchecked) - .filterNot(exclude), - (dependencyJars(dependencyClasspath).value: @sbtUnchecked).filterNot(exclude).toSet, - baseLoader, - runtimeCache, - testCache, - ClasspathUtilities.createClasspathResources(classpath, instance), - taskTemporaryDirectory.value: @sbtUnchecked, - resolvedScope + strategy = classLoaderLayeringStrategy.value: @sbtUnchecked, + si = instance, + fullCP = classpath, + rawRuntimeDependencies = + (dependencyJars(Runtime / dependencyClasspath).value: @sbtUnchecked) + .filterNot(exclude), + allDependencies = + (dependencyJars(dependencyClasspath).value: @sbtUnchecked).filterNot(exclude).toSet, + base = baseLoader, + runtimeCache = runtimeCache, + testCache = testCache, + resources = ClasspathUtilities.createClasspathResources(classpath, instance), + tmp = taskTemporaryDirectory.value: @sbtUnchecked, + scope = resolvedScope ) } new Run(newLoader, trapExit.value) From 8ef5a67b64b04b90703a6a815bd879adcdb89bb5 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 1 Apr 2019 12:11:03 -0700 Subject: [PATCH 3/7] Add better error message if run fails It is possible with the new layering strategies that tests may fail if a java package private class is accessed across classloader layers. This will result in an IllegalAccessError that is hard to debug. With this commit, I add an error message that will be displayed if run throws an IllegalAccessError that suggests that the user try the ScalaInstance layering strategy or the flat layering strategy. --- main/src/main/scala/sbt/Defaults.scala | 31 +++++++++---------- run/src/main/scala/sbt/Run.scala | 19 ++++++++++-- .../package-private/build.sbt | 1 + .../src/main/scala/sbt/classpath/Run.scala | 12 +++++++ .../classloader-cache/package-private/test | 9 ++++++ .../sbt/scriptedtest/ScriptedTests.scala | 1 + .../src/main/scala/sbt/TestFramework.scala | 18 ++++++++++- 7 files changed, 72 insertions(+), 19 deletions(-) create mode 100644 sbt/src/sbt-test/classloader-cache/package-private/build.sbt create mode 100644 sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala create mode 100644 sbt/src/sbt-test/classloader-cache/package-private/test diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 9fc852485..add344fc7 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -1056,22 +1056,21 @@ object Defaults extends BuildCommon { val result = output map { out => out.events.foreach { case (suite, e) => - e.throwables - .collectFirst { - case t - if t - .isInstanceOf[NoClassDefFoundError] && strategy != ClassLoaderLayeringStrategy.Flat => - t - } - .foreach { t => - s.log.error( - s"Test suite $suite failed with $t. This may be due to the ClassLoaderLayeringStrategy" - + s" ($strategy) used by your task. This issue may be resolved by changing the" - + " ClassLoaderLayeringStrategy in your configuration (generally Test or IntegrationTest)," - + "e.g.:\nTest / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat\n" - + "See ClassLoaderLayeringStrategy.scala for the full list of options." - ) - } + if (strategy != ClassLoaderLayeringStrategy.Flat) { + e.throwables + .find { t => + t.isInstanceOf[NoClassDefFoundError] || t.isInstanceOf[IllegalAccessError] + } + .foreach { t => + s.log.error( + s"Test suite $suite failed with $t. This may be due to the ClassLoaderLayeringStrategy" + + s" ($strategy) used by your task. This issue may be resolved by changing the" + + " ClassLoaderLayeringStrategy in your configuration (generally Test or IntegrationTest)," + + " e.g.:\nTest / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat\n" + + "See ClassLoaderLayeringStrategy.scala for the full list of options." + ) + } + } } val summaries = runners map { diff --git a/run/src/main/scala/sbt/Run.scala b/run/src/main/scala/sbt/Run.scala index 754974e1e..ad0496c7a 100644 --- a/run/src/main/scala/sbt/Run.scala +++ b/run/src/main/scala/sbt/Run.scala @@ -93,11 +93,26 @@ class Run(newLoader: Seq[File] => ClassLoader, trapExit: Boolean) extends ScalaR val main = getMainMethod(mainClassName, loader) invokeMain(loader, main, options) } - private def invokeMain(loader: ClassLoader, main: Method, options: Seq[String]): Unit = { + private def invokeMain( + loader: ClassLoader, + main: Method, + options: Seq[String] + ): Unit = { val currentThread = Thread.currentThread val oldLoader = Thread.currentThread.getContextClassLoader currentThread.setContextClassLoader(loader) - try { main.invoke(null, options.toArray[String]); () } finally { + try { main.invoke(null, options.toArray[String]); () } catch { + case t: Throwable => + t.getCause match { + case e: java.lang.IllegalAccessError => + val msg = s"Error running $main.\n$e\n" + + "If using a layered classloader, this can occur if jvm package private classes are " + + "accessed across layers. This can be fixed by changing to the Flat or " + + "ScalaInstance class loader layering strategies." + throw new IllegalAccessError(msg) + case _ => throw t + } + } finally { currentThread.setContextClassLoader(oldLoader) } } diff --git a/sbt/src/sbt-test/classloader-cache/package-private/build.sbt b/sbt/src/sbt-test/classloader-cache/package-private/build.sbt new file mode 100644 index 000000000..68a4b5df0 --- /dev/null +++ b/sbt/src/sbt-test/classloader-cache/package-private/build.sbt @@ -0,0 +1 @@ +libraryDependencies += "org.scala-sbt.ivy" % "ivy" % "2.3.0-sbt-cb9cc189e9f3af519f9f102e6c5d446488ff6832" \ No newline at end of file diff --git a/sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala b/sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala new file mode 100644 index 000000000..de1de4fab --- /dev/null +++ b/sbt/src/sbt-test/classloader-cache/package-private/src/main/scala/sbt/classpath/Run.scala @@ -0,0 +1,12 @@ +package org.apache.ivy.plugins.parser.m2 + +import org.apache.ivy.core.module.descriptor.DefaultDependencyDescriptor + +object Run { + def main(args: Array[String]): Unit = { + new PomModuleDescriptorBuilder.ConfMapper { + override def addMappingConfs(dd: DefaultDependencyDescriptor, isOptional: Boolean): Unit = {} + } + () + } +} \ No newline at end of file diff --git a/sbt/src/sbt-test/classloader-cache/package-private/test b/sbt/src/sbt-test/classloader-cache/package-private/test new file mode 100644 index 000000000..bda116d0c --- /dev/null +++ b/sbt/src/sbt-test/classloader-cache/package-private/test @@ -0,0 +1,9 @@ +-> run + +> set Compile / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.Flat + +> run + +> set Runtime / classLoaderLayeringStrategy := ClassLoaderLayeringStrategy.ScalaInstance + +> run diff --git a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala index 8375bf835..128e794ae 100644 --- a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala +++ b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala @@ -177,6 +177,7 @@ final class ScriptedTests( case "classloader-cache/jni" => LauncherBased // sbt/Package$ case "classloader-cache/library-mismatch" => LauncherBased // sbt/Package$ case "classloader-cache/runtime-layers" => LauncherBased // sbt/Package$ + case "classloader-cache/package-private" => LauncherBased // sbt/Package$ case "compiler-project/dotty-compiler-plugin" => LauncherBased // sbt/Package$ case "compiler-project/run-test" => LauncherBased // sbt/Package$ case "compiler-project/src-dep-plugin" => LauncherBased // sbt/Package$ diff --git a/testing/src/main/scala/sbt/TestFramework.scala b/testing/src/main/scala/sbt/TestFramework.scala index 3f7c68341..21df1cc8f 100644 --- a/testing/src/main/scala/sbt/TestFramework.scala +++ b/testing/src/main/scala/sbt/TestFramework.scala @@ -122,9 +122,25 @@ final class TestRunner( val results = new scala.collection.mutable.ListBuffer[Event] val handler = new EventHandler { def handle(e: Event): Unit = { results += e } } val loggers: Vector[ContentLogger] = listeners.flatMap(_.contentLogger(testDefinition)) + def errorEvents(e: Throwable): Array[sbt.testing.Task] = { + val taskDef = testTask.taskDef + val event = new Event { + val status = Status.Error + val throwable = new OptionalThrowable(e) + val fullyQualifiedName = taskDef.fullyQualifiedName + val selector = new TestSelector(name) + val fingerprint = taskDef.fingerprint + val duration = -1L + } + results += event + Array.empty + } val nestedTasks = try testTask.execute(handler, loggers.map(_.log).toArray) - finally { + catch { + case NonFatal(e) => errorEvents(e) + case e: IllegalAccessError => errorEvents(e) + } finally { loggers.foreach(_.flush()) } val event = TestEvent(results) From a4f1d23d717c6eb2561a3210ab1a203c52d03f3b Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Tue, 2 Apr 2019 16:37:04 -0700 Subject: [PATCH 4/7] Close test and run classloaders It's good practice to call close on a URLClassLoader when we're done with it. --- main/src/main/scala/sbt/Defaults.scala | 9 +++++++-- run/src/main/scala/sbt/Run.scala | 18 +++++++++++++----- sbt/src/sbt-test/project/unified/build.sbt | 5 +++-- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index add344fc7..107ff54b3 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -8,7 +8,7 @@ package sbt import java.io.{ File, PrintWriter } -import java.net.{ URI, URL } +import java.net.{ URI, URL, URLClassLoader } import java.util.Optional import java.util.concurrent.{ Callable, TimeUnit } @@ -787,9 +787,14 @@ object Defaults extends BuildCommon { // ((streams in test, loadedTestFrameworks, testLoader, testGrouping in test, testExecution in test, fullClasspath in test, javaHome in test, testForkedParallel, javaOptions in test) flatMap allTestGroupsTask).value, testResultLogger in (Test, test) :== TestResultLogger.SilentWhenNoTests, // https://github.com/sbt/sbt/issues/1185 test := { + val loader = testLoader.value match { case u: URLClassLoader => Some(u); case _ => None } val trl = (testResultLogger in (Test, test)).value val taskName = Project.showContextKey(state.value).show(resolvedScoped.value) - trl.run(streams.value.log, executeTests.value, taskName) + try { + trl.run(streams.value.log, executeTests.value, taskName) + } finally { + loader.foreach(_.close()) + } }, testOnly := inputTests(testOnly).evaluated, testQuick := inputTests(testQuick).evaluated diff --git a/run/src/main/scala/sbt/Run.scala b/run/src/main/scala/sbt/Run.scala index ad0496c7a..a1aef0668 100644 --- a/run/src/main/scala/sbt/Run.scala +++ b/run/src/main/scala/sbt/Run.scala @@ -10,14 +10,15 @@ package sbt import java.io.File import java.lang.reflect.{ Method, Modifier } import Modifier.{ isPublic, isStatic } + import sbt.internal.inc.classpath.ClasspathUtilities import sbt.internal.inc.ScalaInstance import sbt.internal.util.MessageOnlyException - import sbt.io.Path - import sbt.util.Logger -import scala.util.{ Try, Success, Failure } + +import scala.reflect.internal.util.ScalaClassLoader.URLClassLoader +import scala.util.{ Failure, Success, Try } import scala.util.control.NonFatal import scala.sys.process.Process @@ -90,8 +91,15 @@ class Run(newLoader: Seq[File] => ClassLoader, trapExit: Boolean) extends ScalaR ): Unit = { log.debug(" Classpath:\n\t" + classpath.mkString("\n\t")) val loader = newLoader(classpath) - val main = getMainMethod(mainClassName, loader) - invokeMain(loader, main, options) + try { + val main = getMainMethod(mainClassName, loader) + invokeMain(loader, main, options) + } finally { + loader match { + case u: URLClassLoader => u.close() + case _ => + } + } } private def invokeMain( loader: ClassLoader, diff --git a/sbt/src/sbt-test/project/unified/build.sbt b/sbt/src/sbt-test/project/unified/build.sbt index 966beab44..fd6471758 100644 --- a/sbt/src/sbt-test/project/unified/build.sbt +++ b/sbt/src/sbt-test/project/unified/build.sbt @@ -48,9 +48,10 @@ Provided by: Defined at: \t(sbt.Defaults.testTasks) Defaults.scala:670 Dependencies: -\tTest / executeTests -\tTest / test / streams \tTest / state +\tTest / testLoader +\tTest / test / streams +\tTest / executeTests \tTest / test / testResultLogger Delegates: \tTest / test From 399dd920b0578609936b0d15562672d4dbfd57f3 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Tue, 2 Apr 2019 16:39:05 -0700 Subject: [PATCH 5/7] Set bgCopyClasspath false for shared layer config It isn't possible to share the runtime and test layers correctly with bgCopyClasspath is used because the runtime classpath uses the dependencies copied to the boot directory while the test classpath uses the classes in target and .ivy2. Since this is not the default and users have to opt in to ClassLoaderLayeringStrategy.ShareRuntimeDependenciesLayerWithTestDependencies, I think this is fine. --- .../main/scala/sbt/ClassLoaderLayeringStrategy.scala | 4 +++- main/src/main/scala/sbt/Defaults.scala | 10 +++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/main/src/main/scala/sbt/ClassLoaderLayeringStrategy.scala b/main/src/main/scala/sbt/ClassLoaderLayeringStrategy.scala index 89eae4f05..7245c2e8b 100644 --- a/main/src/main/scala/sbt/ClassLoaderLayeringStrategy.scala +++ b/main/src/main/scala/sbt/ClassLoaderLayeringStrategy.scala @@ -112,7 +112,9 @@ object ClassLoaderLayeringStrategy { * Add the TestDependencies layer on top of the RuntimeDependencies layer on top of the * ScalaInstance layer. This differs from TestDependencies in that it will not reload the * runtime classpath. The drawback to using this is that if the test dependencies evict - * classes provided in the runtime layer, then tests can fail. + * classes provided in the runtime layer, then tests can fail. In order for sharing the runtime + * layer to work, it is necessary to set [[Keys.bgCopyClasspath]] to false. Otherwise the + * runtime and test classpaths are completely different. */ case object ShareRuntimeDependenciesLayerWithTestDependencies extends ScalaInstance diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 107ff54b3..b504dd5c0 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -1781,7 +1781,15 @@ object Defaults extends BuildCommon { // by the test task layers may be cached without evicting the runtime classloader layers. The // cache size should be a multiple of two to support snapshot layers. ClassLoaderCache(4) - ) + ), + bgCopyClasspath in bgRun := { + val old = (bgCopyClasspath in bgRun).value + old && (Test / classLoaderLayeringStrategy).value != ClassLoaderLayeringStrategy.ShareRuntimeDependenciesLayerWithTestDependencies + }, + bgCopyClasspath in bgRunMain := { + val old = (bgCopyClasspath in bgRunMain).value + old && (Test / classLoaderLayeringStrategy).value != ClassLoaderLayeringStrategy.ShareRuntimeDependenciesLayerWithTestDependencies + }, ) lazy val testSettings: Seq[Setting[_]] = configSettings ++ testTasks ++ From 2c19138394df653d859e0eb21441b40c19284c2f Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Tue, 2 Apr 2019 16:44:20 -0700 Subject: [PATCH 6/7] Fix classpath ordering for layered classloaders The order of the classpath was not previously preserved because I converted the runtime and test classpaths to set. I fix that in this commit. --- .../scala/sbt/internal/ClassLoaders.scala | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index 6b3aed0ba..bfcdcad5a 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaders.scala @@ -46,14 +46,14 @@ private[sbt] object ClassLoaders { val si = scalaInstance.value val rawCP = data(fullClasspath.value) val fullCP = if (si.isManagedVersion) rawCP else si.allJars.toSeq ++ rawCP - val exclude = dependencyJars(exportedProducts).value.toSet ++ si.allJars.toSeq + val exclude = dependencyJars(exportedProducts).value.toSet ++ si.allJars buildLayers( strategy = classLoaderLayeringStrategy.value, si = si, fullCP = fullCP, rawRuntimeDependencies = dependencyJars(Runtime / dependencyClasspath).value.filterNot(exclude), - allDependencies = dependencyJars(dependencyClasspath).value.filterNot(exclude).toSet, + allDependencies = dependencyJars(dependencyClasspath).value.filterNot(exclude), base = interfaceLoader, runtimeCache = (Runtime / classLoaderCache).value, testCache = (Test / classLoaderCache).value, @@ -91,17 +91,16 @@ private[sbt] object ClassLoaders { val runtimeCache = (Runtime / classLoaderCache).value val testCache = (Test / classLoaderCache).value val exclude = dependencyJars(exportedProducts).value.toSet ++ instance.allJars + val runtimeDeps = dependencyJars(Runtime / dependencyClasspath).value.filterNot(exclude) + val allDeps = dependencyJars(dependencyClasspath).value.filterNot(exclude) val newLoader = (classpath: Seq[File]) => { buildLayers( strategy = classLoaderLayeringStrategy.value: @sbtUnchecked, si = instance, fullCP = classpath, - rawRuntimeDependencies = - (dependencyJars(Runtime / dependencyClasspath).value: @sbtUnchecked) - .filterNot(exclude), - allDependencies = - (dependencyJars(dependencyClasspath).value: @sbtUnchecked).filterNot(exclude).toSet, + rawRuntimeDependencies = runtimeDeps, + allDependencies = allDeps, base = baseLoader, runtimeCache = runtimeCache, testCache = testCache, @@ -130,7 +129,7 @@ private[sbt] object ClassLoaders { si: ScalaInstance, fullCP: Seq[File], rawRuntimeDependencies: Seq[File], - allDependencies: Set[File], + allDependencies: Seq[File], base: ClassLoader, runtimeCache: ClassLoaderCache, testCache: ClassLoaderCache, @@ -153,30 +152,31 @@ private[sbt] object ClassLoaders { "Flat, ScalaInstance, RuntimeDependencies }" throw new IllegalArgumentException(msg) } + val allDependenciesSet = allDependencies.toSet // The raw declarations are to avoid having to make a dynamic task. The // allDependencies and allTestDependencies create a mutually exclusive list of jar // dependencies for layers 2 and 3. Note that in the Runtime or Compile configs, it // should always be the case that allTestDependencies == Nil - val allTestDependencies = if (layerTestDependencies) allDependencies else Set.empty[File] + val allTestDependencies = if (layerTestDependencies) allDependenciesSet else Set.empty[File] val allRuntimeDependencies = (if (layerDependencies) rawRuntimeDependencies else Nil).toSet + val scalaInstanceLayer = combine(base, loader(si)) // layer 2 - val runtimeDependencies = allDependencies intersect allRuntimeDependencies - val runtimeLayer = - layer(runtimeDependencies.toSeq, loader(si), runtimeCache, resources, tmp) + val runtimeDependencySet = allDependenciesSet intersect allRuntimeDependencies + val runtimeDependencies = rawRuntimeDependencies.filter(runtimeDependencySet) + lazy val runtimeLayer = + if (layerDependencies) + layer(runtimeDependencies, scalaInstanceLayer, runtimeCache, resources, tmp) + else scalaInstanceLayer // layer 3 (optional if testDependencies are empty) - - // The top layer needs to include the interface jar or else the test task cannot be created. - // It needs to be separated from the runtimeLayer or else the runtimeLayer cannot be - // shared between the runtime and test tasks. - val top = combine(base, runtimeLayer) - val testDependencies = allTestDependencies diff runtimeDependencies - val testLayer = layer(testDependencies.toSeq, top, testCache, resources, tmp) + val testDependencySet = allTestDependencies diff runtimeDependencySet + val testDependencies = allDependencies.filter(testDependencySet) + val testLayer = layer(testDependencies, runtimeLayer, testCache, resources, tmp) // layer 4 val dynamicClasspath = - fullCP.filterNot(testDependencies ++ runtimeDependencies ++ si.allJars) + fullCP.filterNot(testDependencySet ++ runtimeDependencies ++ si.allJars) if (dynamicClasspath.nonEmpty) new LayeredClassLoader(dynamicClasspath, testLayer, resources, tmp) else testLayer From 73cfd7c8bdd484b49ff6695ef006f5f7ba995c0b Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 1 Apr 2019 20:29:01 -0700 Subject: [PATCH 7/7] Don't leak the sbt metabuild classpath in run/test Prior to this commit, it was difficult to prevent the sbt metabuild classpath from leaking into the runtime and test classpaths. The biggest issue is that the test-inferface jar was located in the metabuild classpath. We tried to prevent leakage using the DualClassLoader, but this was an ugly solution that did not seem to work reliably. The fix is to modify the actual sbt metabuild classloader provided by the sbt launcher. To do this, I add a new classloader SbtMetaClassLoader that isolates the test-interface jar from the rest of the classpath. I modify xMain to create a new AppConfiguration that uses this new classloader and use reflection to invoke the sbt main method using the new classloader. Not only do I think that this is a much saner solution than DualLoaders, I accidentally fixed #4575 with this change. --- main/src/main/scala/sbt/Main.scala | 43 +++++++++ .../scala/sbt/internal/ClassLoaders.scala | 96 +++++++------------ .../test/scala/sbt/RunFromSourceMain.scala | 2 +- 3 files changed, 79 insertions(+), 62 deletions(-) diff --git a/main/src/main/scala/sbt/Main.scala b/main/src/main/scala/sbt/Main.scala index 1aae438c6..dd58fec86 100644 --- a/main/src/main/scala/sbt/Main.scala +++ b/main/src/main/scala/sbt/Main.scala @@ -8,6 +8,7 @@ package sbt import java.io.{ File, IOException } +import java.lang.reflect.InvocationTargetException import java.net.URI import java.util.concurrent.atomic.AtomicBoolean import java.util.{ Locale, Properties } @@ -27,6 +28,7 @@ import sbt.io._ import sbt.io.syntax._ import sbt.util.{ Level, Logger, Show } import xsbti.compile.CompilerCache +import xsbti.{ AppMain, AppProvider, ComponentProvider, ScalaProvider } import scala.annotation.tailrec import scala.concurrent.ExecutionContext @@ -35,6 +37,47 @@ import scala.util.control.NonFatal /** This class is the entry point for sbt. */ final class xMain extends xsbti.AppMain { def run(configuration: xsbti.AppConfiguration): xsbti.MainResult = { + val modifiedConfiguration = new ModifiedConfiguration(configuration) + val loader = modifiedConfiguration.provider.loader + // No need to memoize the old class loader. It is reset by the launcher anyway. + Thread.currentThread.setContextClassLoader(loader) + val clazz = loader.loadClass("sbt.xMainImpl$") + val instance = clazz.getField("MODULE$").get(null) + val runMethod = clazz.getMethod("run", classOf[xsbti.AppConfiguration]) + try { + runMethod.invoke(instance, modifiedConfiguration).asInstanceOf[xsbti.MainResult] + } catch { + case e: InvocationTargetException => + // This propogates xsbti.FullReload to the launcher + throw e.getCause + } + } + /* + * Replaces the AppProvider.loader method with a new loader that puts the sbt test interface + * jar ahead of the rest of the sbt classpath in the classloading hierarchy. + */ + private class ModifiedConfiguration(val configuration: xsbti.AppConfiguration) + extends xsbti.AppConfiguration { + private[this] val initLoader = configuration.provider.loader + private[this] val scalaLoader = configuration.provider.scalaProvider.loader + private[this] val metaLoader: ClassLoader = SbtMetaBuildClassLoader(scalaLoader, initLoader) + private class ModifiedAppProvider(val appProvider: AppProvider) extends AppProvider { + override def scalaProvider(): ScalaProvider = appProvider.scalaProvider + override def id(): xsbti.ApplicationID = appProvider.id() + override def loader(): ClassLoader = metaLoader + override def mainClass(): Class[_ <: AppMain] = appProvider.mainClass() + override def entryPoint(): Class[_] = appProvider.entryPoint() + override def newMain(): AppMain = appProvider.newMain() + override def mainClasspath(): Array[File] = appProvider.mainClasspath() + override def components(): ComponentProvider = appProvider.components() + } + override def arguments(): Array[String] = configuration.arguments + override def baseDirectory(): File = configuration.baseDirectory + override def provider(): AppProvider = new ModifiedAppProvider(configuration.provider) + } +} +private[sbt] object xMainImpl { + private[sbt] def run(configuration: xsbti.AppConfiguration): xsbti.MainResult = { import BasicCommandStrings.{ DashClient, DashDashClient, runEarly } import BasicCommands.early import BuiltinCommands.defaults diff --git a/main/src/main/scala/sbt/internal/ClassLoaders.scala b/main/src/main/scala/sbt/internal/ClassLoaders.scala index bfcdcad5a..8e9e927f8 100644 --- a/main/src/main/scala/sbt/internal/ClassLoaders.scala +++ b/main/src/main/scala/sbt/internal/ClassLoaders.scala @@ -9,36 +9,20 @@ package sbt package internal import java.io.File -import java.net.URLClassLoader +import java.net.{ URL, URLClassLoader } +import sbt.ClassLoaderLayeringStrategy.{ ScalaInstance => ScalaInstanceLayer, _ } import sbt.Keys._ import sbt.SlashSyntax0._ import sbt.internal.inc.ScalaInstance -import sbt.internal.inc.classpath.{ ClasspathUtilities, DualLoader, NullLoader } +import sbt.internal.inc.classpath.ClasspathUtilities import sbt.internal.util.Attributed import sbt.internal.util.Attributed.data import sbt.io.IO import sbt.librarymanagement.Configurations.{ Runtime, Test } -import PrettyPrint.indent - -import scala.annotation.tailrec -import ClassLoaderLayeringStrategy.{ ScalaInstance => ScalaInstanceLayer, _ } private[sbt] object ClassLoaders { - private[this] lazy val interfaceLoader = - combine( - classOf[sbt.testing.Framework].getClassLoader, - new NullLoader, - toString = "sbt.testing.Framework interface ClassLoader" - ) - private[this] lazy val baseLoader = { - @tailrec - def getBase(classLoader: ClassLoader): ClassLoader = classLoader.getParent match { - case null => classLoader - case loader => getBase(loader) - } - getBase(ClassLoaders.getClass.getClassLoader) - } + private[this] val interfaceLoader = classOf[sbt.testing.Framework].getClassLoader /* * Get the class loader for a test task. The configuration could be IntegrationTest or Test. */ @@ -54,7 +38,6 @@ private[sbt] object ClassLoaders { rawRuntimeDependencies = dependencyJars(Runtime / dependencyClasspath).value.filterNot(exclude), allDependencies = dependencyJars(dependencyClasspath).value.filterNot(exclude), - base = interfaceLoader, runtimeCache = (Runtime / classLoaderCache).value, testCache = (Test / classLoaderCache).value, resources = ClasspathUtilities.createClasspathResources(fullCP, si), @@ -101,7 +84,6 @@ private[sbt] object ClassLoaders { fullCP = classpath, rawRuntimeDependencies = runtimeDeps, allDependencies = allDeps, - base = baseLoader, runtimeCache = runtimeCache, testCache = testCache, resources = ClasspathUtilities.createClasspathResources(classpath, instance), @@ -130,7 +112,6 @@ private[sbt] object ClassLoaders { fullCP: Seq[File], rawRuntimeDependencies: Seq[File], allDependencies: Seq[File], - base: ClassLoader, runtimeCache: ClassLoaderCache, testCache: ClassLoaderCache, resources: Map[String, String], @@ -139,7 +120,7 @@ private[sbt] object ClassLoaders { ): ClassLoader = { val isTest = scope.config.toOption.map(_.name) == Option("test") val raw = strategy match { - case Flat => flatLoader(fullCP, base) + case Flat => flatLoader(fullCP, interfaceLoader) case _ => val (layerDependencies, layerTestDependencies) = strategy match { case ShareRuntimeDependenciesLayerWithTestDependencies if isTest => (true, true) @@ -160,7 +141,7 @@ private[sbt] object ClassLoaders { val allTestDependencies = if (layerTestDependencies) allDependenciesSet else Set.empty[File] val allRuntimeDependencies = (if (layerDependencies) rawRuntimeDependencies else Nil).toSet - val scalaInstanceLayer = combine(base, loader(si)) + val scalaInstanceLayer = new ScalaInstanceLoader(si) // layer 2 val runtimeDependencySet = allDependenciesSet intersect allRuntimeDependencies val runtimeDependencies = rawRuntimeDependencies.filter(runtimeDependencySet) @@ -204,49 +185,42 @@ private[sbt] object ClassLoaders { if (snapshots.isEmpty) jarLoader else cache.get((snapshots, jarLoader, resources, tmp)) } - // Code related to combining two classloaders that primarily exists so the test loader correctly - // loads the testing framework using the same classloader as sbt itself. - private val interfaceFilter = (name: String) => - name.startsWith("org.scalatools.testing.") || name.startsWith("sbt.testing.") || name - .startsWith("java.") || name.startsWith("sun.") - private val notInterfaceFilter = (name: String) => !interfaceFilter(name) - private class WrappedDualLoader( - val parent: ClassLoader, - val child: ClassLoader, - string: => String - ) extends ClassLoader( - new DualLoader(parent, interfaceFilter, _ => false, child, notInterfaceFilter, _ => true) - ) { + private class ScalaInstanceLoader(val instance: ScalaInstance) + extends URLClassLoader(instance.allJars.map(_.toURI.toURL), interfaceLoader) { override def equals(o: Any): Boolean = o match { - case that: WrappedDualLoader => this.parent == that.parent && this.child == that.child - case _ => false + case that: ScalaInstanceLoader => this.instance.allJars.sameElements(that.instance.allJars) + case _ => false } - override def hashCode: Int = (parent.hashCode * 31) ^ child.hashCode - override lazy val toString: String = string + override def hashCode: Int = instance.hashCode + override lazy val toString: String = + s"ScalaInstanceLoader($interfaceLoader, jars = {${instance.allJars.mkString(", ")}})" } - private def combine(parent: ClassLoader, child: ClassLoader, toString: String): ClassLoader = - new WrappedDualLoader(parent, child, toString) - private def combine(parent: ClassLoader, child: ClassLoader): ClassLoader = - new WrappedDualLoader( - parent, - child, - s"WrappedDualLoader(\n parent =\n${indent(parent, 4)}" - + s"\n child =\n${indent(child, 4)}\n)" - ) // helper methods private def flatLoader(classpath: Seq[File], parent: ClassLoader): ClassLoader = new URLClassLoader(classpath.map(_.toURI.toURL).toArray, parent) - // This makes the toString method of the ScalaInstance classloader much more readable, but - // it is not strictly necessary. - private def loader(si: ScalaInstance): ClassLoader = new ClassLoader(si.loader) { - override lazy val toString: String = - "ScalaInstanceClassLoader(\n instance = " + - s"${indent(si.toString.split(",").mkString("\n ", ",\n ", "\n"), 4)}\n)" - // Delegate equals to that.equals in case that is itself some kind of wrapped classloader that - // needs to delegate its equals method to the delegated ClassLoader. - override def equals(that: Any): Boolean = if (that != null) that.equals(si.loader) else false - override def hashCode: Int = si.loader.hashCode +} + +private[sbt] object SbtMetaBuildClassLoader { + private[this] implicit class Ops(val c: ClassLoader) { + def urls: Array[URL] = c match { + case u: URLClassLoader => u.getURLs + case cl => + throw new IllegalStateException(s"sbt was launched with a non URLClassLoader: $cl") + } + } + def apply(libraryLoader: ClassLoader, fullLoader: ClassLoader): ClassLoader = { + val interfaceFilter: URL => Boolean = _.getFile.endsWith("test-interface-1.0.jar") + val (interfaceURL, rest) = fullLoader.urls.partition(interfaceFilter) + val interfaceLoader = new URLClassLoader(interfaceURL, libraryLoader.getParent) { + override def toString: String = s"SbtTestInterfaceClassLoader(${getURLs.head})" + } + val updatedLibraryLoader = new URLClassLoader(libraryLoader.urls, interfaceLoader) { + override def toString: String = s"ScalaClassLoader(jars = {${getURLs.mkString(", ")}}" + } + new URLClassLoader(rest, updatedLibraryLoader) { + override def toString: String = s"SbtMetaBuildClassLoader" + } } } diff --git a/sbt/src/test/scala/sbt/RunFromSourceMain.scala b/sbt/src/test/scala/sbt/RunFromSourceMain.scala index cc34a72b2..b91e610ee 100644 --- a/sbt/src/test/scala/sbt/RunFromSourceMain.scala +++ b/sbt/src/test/scala/sbt/RunFromSourceMain.scala @@ -56,7 +56,7 @@ object RunFromSourceMain { } @tailrec private def launch(conf: AppConfiguration): Option[Int] = - new xMain().run(conf) match { + xMainImpl.run(conf) match { case e: xsbti.Exit => Some(e.code) case _: xsbti.Continue => None case r: xsbti.Reboot => launch(getConf(conf.baseDirectory(), r.arguments()))