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 f2d0515a7..4a6154167 100644 --- a/main-command/src/test/scala/sbt/internal/ClassLoaderCacheTest.scala +++ b/main-command/src/test/scala/sbt/internal/ClassLoaderCacheTest.scala @@ -52,4 +52,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/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 98074f666..8266a2387 100644 --- a/testing/src/test/scala/sbt/JUnitXmlTestsListenerSpec.scala +++ b/testing/src/test/scala/sbt/JUnitXmlTestsListenerSpec.scala @@ -102,4 +102,54 @@ object JUnitXmlTestsListenerSpec extends BasicTestSuite: tempDir.listFiles().foreach(_.delete()) tempDir.delete() + 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