From f059f625f161f40f2fa547632098f59f98316cdf Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 13 May 2019 17:00:11 -0700 Subject: [PATCH 1/3] Add ability to flush ClassLoaderCache Even though the ClassLoaderCache uses soft references, it is still holding references to URLClassLoaders, which, in turn, may be holding open handles to files. We should have a mechanism for explicitly closing these. --- .../inc/classpath/ClassLoaderCache.scala | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala b/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala index 5d0fb84f38..6bfba9cc2e 100644 --- a/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala +++ b/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala @@ -19,9 +19,10 @@ import java.io.File import java.net.URLClassLoader import java.util.HashMap import sbt.io.IO +import scala.util.control.NonFatal // Hack for testing only -final class ClassLoaderCache(val commonParent: ClassLoader) { +final class ClassLoaderCache(val commonParent: ClassLoader) extends AutoCloseable { private[this] val delegate = new HashMap[List[File], Reference[CachedClassLoader]] @@ -52,6 +53,10 @@ final class ClassLoaderCache(val commonParent: ClassLoader) { getFromReference(files, tstamps, delegate.get(files), mkLoader) } + override def close(): Unit = { + delegate.values.forEach { _.get.close() } + } + private[this] def getFromReference( files: List[File], stamps: List[Long], @@ -91,4 +96,11 @@ private[sbt] final class CachedClassLoader( val loader: ClassLoader, val files: List[File], val timestamps: List[Long] -) +) extends AutoCloseable { + override def close(): Unit = loader match { + case a: AutoCloseable => + try a.close() + catch { case NonFatal(e) => e.printStackTrace() } + case _ => + } +} From 271e58740d5afd5990cdffda2d2d8f8303ef4588 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 13 May 2019 17:43:37 -0700 Subject: [PATCH 2/3] Remove comment The ClassLoaderCache is used in may places so it is clearly cannot just be considered a "Hack for testing only". --- .../main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala b/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala index 6bfba9cc2e..962b1ca662 100644 --- a/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala +++ b/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala @@ -21,7 +21,6 @@ import java.util.HashMap import sbt.io.IO import scala.util.control.NonFatal -// Hack for testing only final class ClassLoaderCache(val commonParent: ClassLoader) extends AutoCloseable { private[this] val delegate = new HashMap[List[File], Reference[CachedClassLoader]] From 6352551f125b2d069028213e92c14f3bbc16ac39 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Thu, 16 May 2019 12:21:46 -0700 Subject: [PATCH 3/3] Make ClassLoaderCache extendable by delegation We are having a lot of problems in sbt 1.3.0-RC1 with metaspace utilization. Leaking classlaoders in the ClassLoaderCache is a primary culprit. I want to be able to modify the ClassLoaderCache implementation in sbt so that we can manually close it in a number of situations (such as when the user runs '++' directly or indirectly. To do this, I define a trait, AbstractClassLoaderCache that specifies the public api of ClassLoaderCache and then refactor ClassLoaderCache so that the primary constructor takes an instance of AbstractClassLoaderCache and delegates to it. Finally, I provide a default implementation that is identical to the old version. --- .../inc/classpath/ClassLoaderCache.scala | 44 +++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala b/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala index 962b1ca662..0f580e6ed2 100644 --- a/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala +++ b/internal/zinc-classpath/src/main/scala/sbt/internal/inc/classpath/ClassLoaderCache.scala @@ -14,14 +14,51 @@ package internal package inc package classpath -import java.lang.ref.{ Reference, SoftReference } import java.io.File +import java.lang.ref.{ Reference, SoftReference } import java.net.URLClassLoader import java.util.HashMap + import sbt.io.IO + import scala.util.control.NonFatal -final class ClassLoaderCache(val commonParent: ClassLoader) extends AutoCloseable { +trait AbstractClassLoaderCache extends AutoCloseable { + def commonParent: ClassLoader + def apply(files: List[File]): ClassLoader + + /** + * Returns a ClassLoader, as created by `mkLoader`. + * + * The returned ClassLoader may be cached from a previous call if the last modified time of all `files` is unchanged. + * This method is thread-safe. + */ + def cachedCustomClassloader( + files: List[File], + mkLoader: () => ClassLoader + ): ClassLoader +} +final class ClassLoaderCache(private val abstractClassLoaderCache: AbstractClassLoaderCache) + extends AutoCloseable { + def this(commonParent: ClassLoader) = this(new ClassLoaderCacheImpl(commonParent)) + def commonParent: ClassLoader = abstractClassLoaderCache.commonParent + override def close(): Unit = abstractClassLoaderCache.close() + def apply(files: List[File]): ClassLoader = abstractClassLoaderCache.apply(files) + + /** + * Returns a ClassLoader, as created by `mkLoader`. + * + * The returned ClassLoader may be cached from a previous call if the last modified time of all `files` is unchanged. + * This method is thread-safe. + */ + def cachedCustomClassloader( + files: List[File], + mkLoader: () => ClassLoader + ): ClassLoader = abstractClassLoaderCache.cachedCustomClassloader(files, mkLoader) +} + +private final class ClassLoaderCacheImpl(val commonParent: ClassLoader) + extends AbstractClassLoaderCache { private[this] val delegate = new HashMap[List[File], Reference[CachedClassLoader]] @@ -53,7 +90,8 @@ final class ClassLoaderCache(val commonParent: ClassLoader) extends AutoCloseabl } override def close(): Unit = { - delegate.values.forEach { _.get.close() } + delegate.values.forEach(v => Option(v.get).foreach(_.close())) + delegate.clear() } private[this] def getFromReference(