Use layered class loaders rather than "shared" class loader hack for class loader isolation #5155

alexarchambault · 2025-05-19T16:37:16Z

This PR makes Mill start its daemon with the help of coursier bootstraps, to isolate some Mill dependencies. These bootstraps are generated so that they load Mill using three class loaders:

a first one with org.scala-sbt:compiler-interface and org.scala-sbt:test-interface
a second one, with the first as parent, that loads the core-api module of Mill (alongside its dependencies)
a third one, loading the rest of the Mill class path

The first class loader is the parent of the second one, and the second one is the parent of the third one.

When loading scala-compiler JARs (in JvmWorkerImpl#scalaCompilerCache) or test frameworks (like in TestModule#bspBuildTargetScalaTestClasses, to discover tests from within the Mill process, but it's also done in other places), Mill uses the first class loader as a parent loader for the class loader that loads scala-compiler or the class path that has tests. That way, the compiler-interface and test-interface classes are shared between Mill's internals and scalac or the test class path.

When loading the build class path in MillBuildBootstrap#processRunClasspath, Mill uses the second class loader, so that the core-api class path is shared between Mill's internals and the user build, but the rest of the Mill class path is hidden from user builds.

This has two benefits, compared to what's done without this PR:

avoiding mistakes or issues that can arise with the use of package prefixes to hide or show classes (like done here)
make the class loader hierarchy more "standard", consisting solely of URLClassLoaders (and the JVM's app and bootstrap loaders), so that it's easier to inspect by users if they need to

Initially, these development were motivated by suspicious things seen around SystemStreams.ThreadLocalStreams.current involving thread local variables, in #5154, but it turns out the changes in the PR here are not necessary to address these issues.

So this PR only makes things safer or more standard, but it isn't required for any feature as I initially thought. If you've seen anything suspicious with the handling of class loaders, it might be helpful…

Seems integration.ide[bsp-server].packaged.server.testForked is run on CI, but not the local one

lihaoyi · 2025-05-19T17:12:18Z

core/util/src/mill/util/Jvm.scala

  def createClassLoader(
      classPath: Iterable[os.Path],
-      parent: ClassLoader = null,
+      parent: ClassLoader = Thread.currentThread().getContextClassLoader,


I think better to keep this default as is; flipping the default will likely cause a lot of confusion since the code will all compile and run but possibly fail with subtle classloading issues

I'm also in favor of avoiding the context classloader if we can specify one explicitly.
Since this PR also adds createIsolatedClassLoader, we should just remove the default argument here.

lihaoyi · 2025-05-19T17:14:25Z

runner/client/src/mill/client/ServerLauncher.java

+      if (locks.daemonLock.probe()) serverProcess = initServer(daemonDir, locks);
+      while (locks.daemonLock.probe()) {
+        if (serverProcess != null && !serverProcess.isAlive()) {
+          System.err.println("Mill server exited!");


I assume this is just debugging logging that should be cleaned up?

lihaoyi · 2025-05-19T17:16:44Z

runner/daemon/src/mill/daemon/MillBuildBootstrap.scala

-            sharedLoader = classOf[MillBuildBootstrap].getClassLoader,
-            sharedPrefixes = Seq("java.", "javax.", "scala.", "mill.api")
-          )
+          val hasLayeredClassLoader =


These conditionals are new, and seem to against the goal of making the classloader hierarchy easier to understand. Why do we need them now, if we did not need them before?

lihaoyi · 2025-05-19T17:20:33Z

Is it possible to set up the layered classloaders without using coursier's bootstrap infrastucture? It seems to be straightforward to do it "manually", as we do a similar thing in TestRunnerMain.java, and if it's not difficult it would be nice to have the logic encapsulated here rather than calling out to a third-party library

Overall the high-level goal of this PR seems reasonable, but the details of the PR seem to contradict the notion that this makes the classloader setup cleaner or easier to understand. At first glance, this PR seems to introduce a lot more complexity (including usage of arbitrarily-complex third-party library logic in coursier-bootstrap) as well as more confusion in the Mill codebase (e.g. the if conditionals scattered throughout the codebase). Not sure if that is fundamental or incidental, but if it's incidental perhaps after some cleanup we could demonstrate an improvement in clarity over the status quo

lefou

I stopped review after I realized, that the direction of the refactorings is to prefer the context classloader. I don't like it. It seem there is no real need, but using the context classloader is a can of worms. We are in the fortunate current situation that we exactly know which classloader we use in all code locations. We should not give up this now. It's a valuable quality. Even if you provide a stronger motivation to not usegetClass.getClassloader, I'd argue that we should use some dedicated API to provide the "correct" classloader instead of just using the context classloader, which is some uncontrollable wishy-washy resource.

lefou · 2025-05-20T09:12:14Z

contrib/scoverage/src/mill/contrib/scoverage/ScoverageReportWorker.scala

-          classpath.map(_.path).toVector,
-          getClass.getClassLoader
-        ) { cl =>
+        mill.util.Jvm.withClassLoader(classpath.map(_.path).toVector) { cl =>


This looks suspicious. We should not refactor an explicitly known classloader into some implicitly known. This reduces clarity. Although there is a concept of a context class loader, we should not rely on it if we can manage it directly.

lefou · 2025-05-20T09:14:58Z

core/util/src/mill/util/Jvm.scala

  def createClassLoader(
      classPath: Iterable[os.Path],
-      parent: ClassLoader = null,
+      parent: ClassLoader = Thread.currentThread().getContextClassLoader,


I'm also in favor of avoiding the context classloader if we can specify one explicitly.
Since this PR also adds createIsolatedClassLoader, we should just remove the default argument here.

lefou · 2025-05-20T09:15:41Z

core/util/src/mill/util/Jvm.scala

+   */
+  def createIsolatedClassLoader(
+      classPath: Iterable[os.Path],
+      label: String = null


Please add some docs, since null is always required to be explained.

lefou · 2025-05-20T09:17:53Z

core/util/src/mill/util/Jvm.scala

@@ -241,7 +260,7 @@ object Jvm {
   */
  def withClassLoader[T](
      classPath: Iterable[os.Path],
-      parent: ClassLoader = null,
+      parent: ClassLoader = Thread.currentThread().getContextClassLoader,


We should remove the default. Using the context classloader should be a conscious decision at the use site.

lefou · 2025-05-20T09:19:11Z

libs/kotlinlib/src/mill/kotlinlib/KotlinWorkerManager.scala

@@ -15,7 +15,7 @@ class KotlinWorkerFactory()(implicit ctx: TaskCtx)
    extends CachedFactory[Seq[os.Path], (URLClassLoader, KotlinWorker)] {

  def setup(key: Seq[os.Path]) = {
-    val cl = mill.util.Jvm.createClassLoader(key, getClass.getClassLoader)
+    val cl = mill.util.Jvm.createClassLoader(key)


Keep it explicitly.

This makes the Mill client print the daemon stdout and stderr if the daemon crashes before we even connect to it (and redirect its streams). This has been useful to me when hacking on #5155, but this should also be useful if anything goes silently wrong when fetching the daemon class path, and it fails to start early because of that. Currently, in such a scenario, the client keep waiting indefinitely, and users have no idea what's going on. With this PR, the client prints things like this straightaway, which should help users debug things or report an issue: ```text Mill daemon exited unexpectedly! No daemon stdout Daemon stderr: Exception in thread "main" java.lang.RuntimeException: Mill daemon early crash requested at scala.sys.package$.error(package.scala:27) at mill.daemon.MillDaemonMain$.main(MillDaemonMain.scala:15) at mill.daemon.MillDaemonMain.main(MillDaemonMain.scala) ```

alexarchambault added 6 commits May 19, 2025 11:02

Fix integration.ide[bsp-server].local.server.testForked

975da0a

Seems integration.ide[bsp-server].packaged.server.testForked is run on CI, but not the local one

Start Mill with a layered class loader

2e80ba6

Exit early if the server fails to start

3504423

Don't mask exceptions when class loader isn't closed yet

98e7579

Set context class loader when running tasks

5f20674

Add more specialized class loader method, better default parent

3f7c1ce

alexarchambault marked this pull request as ready for review May 19, 2025 16:38

lihaoyi reviewed May 19, 2025

View reviewed changes

alexarchambault mentioned this pull request May 19, 2025

Prefix logging for BSP #5154

Closed

lihaoyi reviewed May 19, 2025

View reviewed changes

lefou reviewed May 20, 2025

View reviewed changes

alexarchambault mentioned this pull request Jun 6, 2025

Exit client early if the daemon fails to start #5267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use layered class loaders rather than "shared" class loader hack for class loader isolation #5155

Use layered class loaders rather than "shared" class loader hack for class loader isolation #5155

Uh oh!

alexarchambault commented May 19, 2025 •

edited

Loading

Uh oh!

lihaoyi May 19, 2025

Uh oh!

lefou May 20, 2025

Uh oh!

lihaoyi May 19, 2025

Uh oh!

lihaoyi May 19, 2025

Uh oh!

lihaoyi commented May 19, 2025

Uh oh!

lefou left a comment

Uh oh!

lefou May 20, 2025

Uh oh!

lefou May 20, 2025

Uh oh!

lefou May 20, 2025

Uh oh!

lefou May 20, 2025

Uh oh!

lefou May 20, 2025

Uh oh!

Uh oh!

Uh oh!

Use layered class loaders rather than "shared" class loader hack for class loader isolation #5155

Are you sure you want to change the base?

Use layered class loaders rather than "shared" class loader hack for class loader isolation #5155

Uh oh!

Conversation

alexarchambault commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lihaoyi commented May 19, 2025

Uh oh!

lefou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexarchambault commented May 19, 2025 •

edited

Loading