diff --git a/main-actions/src/main/scala/sbt/internal/testing/TestRecap.scala b/main-actions/src/main/scala/sbt/internal/testing/TestRecap.scala index ee4557d16..1dd2adbb8 100644 --- a/main-actions/src/main/scala/sbt/internal/testing/TestRecap.scala +++ b/main-actions/src/main/scala/sbt/internal/testing/TestRecap.scala @@ -45,7 +45,12 @@ import sbt.util.Logger */ private[sbt] object TestRecap: - /** A single failed test task contributing to the recap. */ + /** + * A single failed test task contributing to the recap. + * + * `testOutput` is sanitized by [[collect]]: its `SuiteResult.throwables` are dropped so the + * recap cannot pin the test class loader. See [[collect]] for why. + */ final case class Failure(taskName: String, testOutput: Option[Tests.Output]) /** @@ -53,6 +58,13 @@ private[sbt] object TestRecap: * aggregated run that produced at least one `TestsFailedException`. * Monotonic-latest-failure semantics: never cleared on success, only * overwritten by the next failure. + * + * Note for consumers: the `SuiteResult.throwables` reachable from these + * failures are always empty -- [[collect]] strips them so the recap cannot + * pin the test class loader. An empty `throwables` here therefore means + * "not retained", NOT "no exception was thrown". The real throwables live on + * the `TestsFailedException` in the `Incomplete` tree, which is where error + * reporting reads them from. */ val recapKey: AttributeKey[Vector[Failure]] = AttributeKey[Vector[Failure]]( "testRecap", @@ -69,17 +81,44 @@ private[sbt] object TestRecap: * Identity-deduplicated via `Incomplete.allExceptions` (which uses an * `IDSet[Throwable]` internally), so a single failing task shared across * multiple Incomplete paths in a DAG is counted once. + * + * The retained `Tests.Output` is stripped of `SuiteResult.throwables`: the + * recap only renders names and counts, but a test-thrown Throwable's + * backtrace pins the `Class` objects of every frame, and a `Class` strongly + * references its defining class loader. Since this data is stashed on + * `State.attributes` where it outlives the command, retaining the throwables + * would keep the test class loader -- and its open jar handles -- alive for + * the rest of the session. On Windows those handles make the cached jars + * undeletable (e.g. by `clearCaches`). */ def collect(i: Incomplete): Vector[Failure] = Incomplete .allExceptions(i) .iterator .flatMap { - case e: TestsFailedException => Some(Failure(e.taskName, e.testOutput)) - case _ => None + case e: TestsFailedException => + Some(Failure(e.taskName, e.testOutput.map(dropThrowables))) + case _ => None } .toVector + private def dropThrowables(o: Tests.Output): Tests.Output = + o.copy(events = o.events.view.mapValues(dropThrowables).toMap) + + private def dropThrowables(s: SuiteResult): SuiteResult = + if s.throwables.isEmpty then s + else + new SuiteResult( + s.result, + s.passedCount, + s.failureCount, + s.errorCount, + s.skippedCount, + s.ignoredCount, + s.canceledCount, + s.pendingCount, + ) + /** * The rendered recap as a sequence of `\n`-free lines. Failures are * sorted by `taskName` (lexicographically; empty task names last) for diff --git a/main-actions/src/test/scala/sbt/internal/testing/TestRecapTest.scala b/main-actions/src/test/scala/sbt/internal/testing/TestRecapTest.scala index 685112dd9..c8be710d6 100644 --- a/main-actions/src/test/scala/sbt/internal/testing/TestRecapTest.scala +++ b/main-actions/src/test/scala/sbt/internal/testing/TestRecapTest.scala @@ -50,6 +50,52 @@ object TestRecapTest extends verify.BasicTestSuite: override def log(level: sbt.util.Level.Value, msg: => String): Unit = lines += level.toString -> msg + test("collect strips SuiteResult.throwables so the recap cannot pin the test classloader") { + // A test-thrown Throwable's backtrace pins the Class objects of every frame, and a Class + // strongly references its defining classloader; the recap is stashed on State.attributes, + // so retaining them would keep the test classloader (and its jar handles) alive. + val thrown = new AssertionError("boom") + val withThrowables = + new SuiteResult(TestResult.Failed, 0, 1, 0, 0, 0, 0, 0, thrown :: Nil) + val i = incompleteOf( + new TestsFailedException( + "a / Test / test", + Some(output(TestResult.Failed, "AFailing" -> withThrowables)) + ) + ) + val collected = TestRecap.collect(i) + val retained = collected.head.testOutput.get.events("AFailing") + assert(retained.throwables.isEmpty, "throwables should be dropped from the retained output") + // Everything the recap actually renders must survive. + assert(retained.result == TestResult.Failed) + assert(retained.failureCount == 1) + assert(collected.head.testOutput.get.overall == TestResult.Failed) + } + + test("collect sanitizes a copy and leaves the source exception's throwables intact") { + // The stripping above must not mutate the TestsFailedException in the Incomplete tree: + // that exception is what error reporting and the ClassLoaderLayeringStrategy diagnostic + // in Defaults read, and both need the real throwables. Only the copy parked on + // State.attributes is sanitized. + val thrown = new AssertionError("boom") + val source = new TestsFailedException( + "a / Test / test", + Some( + output( + TestResult.Failed, + "AFailing" -> new SuiteResult(TestResult.Failed, 0, 1, 0, 0, 0, 0, 0, thrown :: Nil) + ) + ) + ) + val collected = TestRecap.collect(incompleteOf(source)) + assert(collected.head.testOutput.get.events("AFailing").throwables.isEmpty) + val original = source.testOutput.get.events("AFailing").throwables + assert( + original == (thrown :: Nil), + s"collect must not strip the source exception, but its throwables became: $original" + ) + } + test("collect picks up TestsFailedException payloads from the Incomplete tree") { val i = incompleteOf( failure("a / Test / test", TestResult.Failed, "AFailing"), diff --git a/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala b/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala index c77913911..1bc7cb979 100644 --- a/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala +++ b/main-command/src/main/scala/sbt/internal/classpath/ClassLoaderCache.scala @@ -77,12 +77,34 @@ private[sbt] class ClassLoaderCache( new java.util.concurrent.ConcurrentHashMap[Key, Reference[ClassLoader]]() private val referenceQueue = new ReferenceQueue[ClassLoader] + /* + * Loaders evicted from `delegate` by clearExpiredLoaders are no longer reachable from the + * map, so clear()/close() alone can never close them. Nor can the cleanup thread: once the + * entry is removed, the Reference object itself becomes unreachable, and an unreachable + * Reference is never enqueued on the ReferenceQueue. They would linger with open jar handles + * for the life of the JVM. On Windows those handles make the underlying jars undeletable + * (e.g. clearCaches cannot delete cas blobs the loaders still reference). Track evicted + * loaders weakly so clear() can close them deterministically; weak keys preserve the + * metaspace-pressure design above by adding no strong retention of their own. + */ + private val retired = + java.util.Collections.synchronizedMap(new java.util.WeakHashMap[ClassLoader, java.lang.Boolean]) + private def clearExpiredLoaders(): Unit = lock.synchronized { val clear = (k: Key, ref: Reference[ClassLoader]) => { ref.get() match { case w: WrappedLoader => w.invalidate() case _ => } + ref match { + case ClassLoaderReference(_, underlying) => + retired.put(underlying, java.lang.Boolean.TRUE) + case r => + r.get() match { + case null => + case loader => retired.put(loader, java.lang.Boolean.TRUE) + } + } delegate.remove(k) () } @@ -109,6 +131,7 @@ private[sbt] class ClassLoaderCache( referenceQueue.remove(1000) match { case ClassLoaderReference(key, classLoader) => close(classLoader) + retired.remove(classLoader) delegate.remove(key) () case _ => @@ -148,6 +171,10 @@ private[sbt] class ClassLoaderCache( * handle from being modified. On linux and mac, we probably leak some file descriptors but it's * fairly uncommon for sbt to run out of file descriptors. * + * Loaders evicted by clearExpiredLoaders (as opposed to by garbage collection) are a separate + * case: they are removed from `delegate`, so neither clear()/close() nor the reference-queue + * cleanup thread can reach them. Those are tracked weakly in `retired` and closed by + * clear()/close(), so their handles are released deterministically rather than never. */ private val metaspaceIsLimited = ManagementFactory.getMemoryPoolMXBeans.asScala @@ -230,6 +257,12 @@ private[sbt] class ClassLoaderCache( } } delegate.clear() + /* Also close loaders that were evicted from the delegate map but never closed (see + * `retired`); WeakHashMap iteration requires holding its monitor, so snapshot the keys + * first and close outside the lock. */ + val evicted = retired.synchronized(new java.util.ArrayList(retired.keySet()).asScala.toList) + evicted.foreach(close) + retired.clear() } /** diff --git a/main-command/src/test/scala/sbt/internal/ClassLoaderCacheTest.scala b/main-command/src/test/scala/sbt/internal/ClassLoaderCacheTest.scala index b63a4cf85..fcb12471a 100644 --- a/main-command/src/test/scala/sbt/internal/ClassLoaderCacheTest.scala +++ b/main-command/src/test/scala/sbt/internal/ClassLoaderCacheTest.scala @@ -51,4 +51,31 @@ object ClassLoaderCacheTest extends BasicTestSuite: Predef.assert(cache.get(jarClassPath) == secondLoader) Predef.assert(cache.get(jarClassPath) != initLoader) + test("Loaders evicted by a newer timestamp should be closed by clear()"): + IO.withTemporaryDirectory: dir => + val entry = "leak-test-resource.txt" + val jar = dir.toPath.resolve("evicted.jar").toFile + Using.resource(new java.util.jar.JarOutputStream(new java.io.FileOutputStream(jar))): out => + out.putNextEntry(new java.util.zip.ZipEntry(entry)) + out.write("hello".getBytes("UTF-8")) + out.closeEntry() + + withCache: cache => + val classPath = jar :: Nil + val first = cache.get(classPath) + Predef.assert(first.getResource(entry) != null, "jar resource should load before eviction") + + // Bumping the timestamp makes a new Key, and addLoader calls clearExpiredLoaders after + // inserting it, so `first`'s entry is evicted from the delegate map here. Once evicted + // it is unreachable from the map, so only the `retired` set can still close it. + IO.setModifiedTimeOrFalse(jar, System.currentTimeMillis + 5000L) + val second = cache.get(classPath) + Predef.assert(first != second, "a newer timestamp should produce a new loader") + + cache.clear() + Predef.assert( + first.getResource(entry) == null, + "clear() should have closed the evicted loader, releasing its jar handle" + ) + end ClassLoaderCacheTest diff --git a/sbt-app/src/sbt-test/tests/multi-failure-recap/project/Checks.scala b/sbt-app/src/sbt-test/tests/multi-failure-recap/project/Checks.scala index cca897add..c2c798448 100644 --- a/sbt-app/src/sbt-test/tests/multi-failure-recap/project/Checks.scala +++ b/sbt-app/src/sbt-test/tests/multi-failure-recap/project/Checks.scala @@ -39,6 +39,16 @@ object Checks { val failedSuites = f.testOutput.get.events.values.count: s => s.result == sbt.protocol.testing.TestResult.Failed assert(failedSuites >= 1, s"${f.taskName} has no failed suite: ${f.testOutput.get.events}") + // The retained output must not carry live test-thrown Throwables: their backtraces pin + // the test classloader (and its open jar handles) for as long as the recap sits on + // State.attributes, which is the whole session. The recap only renders names and counts. + f.testOutput.get.events.foreach { case (suiteName, s) => + assert( + s.throwables.isEmpty, + s"${f.taskName} suite $suiteName retained ${s.throwables.size} throwable(s): " + + s.throwables.map(_.getClass.getName).mkString(", ") + ) + } } val lines = TestRecap.render(recap) assert( diff --git a/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala b/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala index bfb2adacd..c84ebef38 100644 --- a/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala +++ b/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala @@ -15,6 +15,7 @@ import java.time.format.DateTimeFormatter import java.time.temporal.ChronoUnit import java.util.Hashtable import java.util.concurrent.TimeUnit.NANOSECONDS +import java.util.concurrent.atomic.AtomicReference import scala.collection.mutable.ListBuffer import scala.util.Properties @@ -196,13 +197,31 @@ class JUnitXmlTestsListener(val targetDir: File, legacyTestReport: Boolean, logg } } + /** + * A mutable cell holding the suite that is currently running on a thread. + * + * The cell exists purely so the suite can be released deterministically. `testSuite` below + * is an [[InheritableThreadLocal]], so every thread created while a suite is running -- for + * example a pooled worker spawned by an async test framework -- receives a copy of the + * *reference* to this cell at construction time. `ThreadLocal.remove()` only clears the + * calling thread's entry, so those inherited copies would otherwise pin the `TestSuite`, its + * buffered events, and through them the test class loader (and its open jar handles) for the + * remaining life of the JVM. Clearing the cell severs the reference for the owning thread and + * every thread that inherited it at once. + */ + private final class SuiteRef(initial: Option[TestSuite]) { + private val ref = new AtomicReference(initial) + def current: Option[TestSuite] = ref.get() + def clear(): Unit = ref.set(None) + } + /** The currently running test suite */ - private val testSuite = new InheritableThreadLocal[Option[TestSuite]] { - override def initialValue(): Option[TestSuite] = None + private val testSuite = new InheritableThreadLocal[SuiteRef] { + override def initialValue(): SuiteRef = new SuiteRef(None) } private def withTestSuite[T](f: TestSuite => T): T = - testSuite.get().map(f).getOrElse(sys.error("no test suite")) + testSuite.get().current.map(f).getOrElse(sys.error("no test suite")) /** Creates the output Dir */ override def doInit(): Unit = { @@ -212,14 +231,29 @@ class JUnitXmlTestsListener(val targetDir: File, legacyTestReport: Boolean, logg /** * Starts a new, initially empty Suite with the given name. */ - override def startGroup(name: String): Unit = testSuite.set(Some(new TestSuite(name))) + override def startGroup(name: String): Unit = + testSuite.set(new SuiteRef(Some(new TestSuite(name)))) /** * Adds all details for the given even to the current suite. + * + * Events that arrive after the suite has been written are dropped. Test frameworks may call + * the event handler from threads they spawned during the run (see `TestFramework.TestRunner`), + * and such a thread can report after `writeSuite` has already emitted the XML. Before the + * suite was released those late events were appended to an already-written suite, so they + * were discarded in practice; dropping them here keeps that outcome without turning every + * late event into an error line via `TestFramework.safeForeach`. */ - override def testEvent(event: TestEvent): Unit = for (e <- event.detail) { - withTestSuite(_.addEvent(e)) - } + override def testEvent(event: TestEvent): Unit = + testSuite.get().current match { + case Some(suite) => for (e <- event.detail) suite.addEvent(e) + case None => + if (logger != null) { + logger.debug( + s"ignoring ${event.detail.size} test event(s) reported after the suite was written" + ) + } else () + } /** * called for each class or equivalent grouping We map one group to one Testsuite, so for each @@ -283,6 +317,13 @@ class JUnitXmlTestsListener(val targetDir: File, legacyTestReport: Boolean, logg } val testSuiteResult = withTestSuite(_.stop()) XML.save(file, testSuiteResult, "UTF-8", xmlDecl = true, null) + /* Order matters: `clear()` releases the suite for this thread *and* for every thread that + * inherited the cell, which `remove()` cannot reach. `remove()` then drops this thread's + * own entry. Without the `clear()` the suite -- and through its buffered events the test + * class loader with its open jar handles -- would stay reachable from pooled worker threads + * for the life of the JVM. + */ + testSuite.get().clear() testSuite.remove() } diff --git a/testing/src/main/scala/sbt/TestReportListener.scala b/testing/src/main/scala/sbt/TestReportListener.scala index 2b5a9ee60..3870e1786 100644 --- a/testing/src/main/scala/sbt/TestReportListener.scala +++ b/testing/src/main/scala/sbt/TestReportListener.scala @@ -44,6 +44,20 @@ trait TestsListener extends TestReportListener { /** * Provides the overall `result` of a group of tests (a suite) and test counts for each result type. + * + * @param throwables + * The exceptions thrown by the tests in this suite, as live objects. They are needed as objects + * rather than as rendered text because the ClassLoaderLayeringStrategy diagnostic in `Defaults` + * inspects them with `isInstanceOf` and walks `getCause` to recognise `NoClassDefFoundError` and + * friends. + * + * RETENTION HAZARD: a `Throwable`'s backtrace references the `Class` objects of every frame, and + * a `Class` strongly references its defining class loader. Holding a `SuiteResult` with a + * non-empty `throwables` therefore keeps the test class loader -- and every jar handle it has + * open -- alive. That is fine for the duration of the test task, which is where these are + * consumed, but anything that outlives the task must drop them first. `TestRecap.collect` does + * exactly that before stashing a copy on `State.attributes`; see the note on `TestRecap.recapKey`. + * On Windows a leaked handle makes the underlying jar undeletable (e.g. by `clearCaches`). */ private[sbt] final class SuiteResult( val result: TestResult, diff --git a/testing/src/test/scala/sbt/JUnitXmlTestsListenerSpec.scala b/testing/src/test/scala/sbt/JUnitXmlTestsListenerSpec.scala index 959b5f538..58fc6ce88 100644 --- a/testing/src/test/scala/sbt/JUnitXmlTestsListenerSpec.scala +++ b/testing/src/test/scala/sbt/JUnitXmlTestsListenerSpec.scala @@ -88,4 +88,54 @@ object JUnitXmlTestsListenerSpec extends BasicTestSuite: val xmlFile = new File(tempDir, "TEST-TestSuite.xml") assert(xmlFile.exists(), "XML file should be created even when logger is null") + test("JUnitXmlTestsListener should release the suite from threads that inherited it"): + IO.withTemporaryDirectory: tempDir => + val listener = new JUnitXmlTestsListener(tempDir, false, null) + listener.doInit() + + def event(name: String) = new TEvent: + def fullyQualifiedName = s"InheritSuite.$name" + def duration() = 1L + def status = TStatus.Success + def fingerprint = null + def selector = new TestSelector(name) + def throwable = new OptionalThrowable() + + listener.startGroup("InheritSuite") + + // A thread created *while* the suite is set inherits the suite cell, standing in for a + // pooled worker spawned by an async test framework during the run. + val suiteWritten = new java.util.concurrent.CountDownLatch(1) + val childDone = new java.util.concurrent.CountDownLatch(1) + val endGroupOutcome = new AtomicReference[Option[Throwable]](None) + val testEventOutcome = new AtomicReference[Option[Throwable]](None) + val child = new Thread(() => + suiteWritten.await() + // The inherited cell must no longer reach a TestSuite, so the strict path fails... + endGroupOutcome.set( + scala.util.Try(listener.endGroup("InheritSuite", TestResult.Passed)).failed.toOption + ) + // ...while a late event is dropped rather than raised. + testEventOutcome.set( + scala.util.Try(listener.testEvent(sbt.TestEvent(Seq(event("late"))))).failed.toOption + ) + childDone.countDown() + ) + child.setDaemon(true) + child.start() + + listener.testEvent(sbt.TestEvent(Seq(event("testMethod")))) + listener.endGroup("InheritSuite", TestResult.Passed) + suiteWritten.countDown() + assert(childDone.await(30, java.util.concurrent.TimeUnit.SECONDS), "child thread timed out") + + assert( + endGroupOutcome.get().isDefined, + "a thread that inherited the suite could still reach it after writeSuite" + ) + assert( + testEventOutcome.get().isEmpty, + s"a late test event should be dropped, but threw: ${testEventOutcome.get()}" + ) + end JUnitXmlTestsListenerSpec