Use layered class loaders rather than "shared" class loader hack for class loader isolation #5155
Conversation
Seems integration.ide[bsp-server].packaged.server.testForked is run on CI, but not the local one
| def createClassLoader( | ||
| classPath: Iterable[os.Path], | ||
| parent: ClassLoader = null, | ||
| parent: ClassLoader = Thread.currentThread().getContextClassLoader, |
There was a problem hiding this comment.
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
| if (locks.daemonLock.probe()) serverProcess = initServer(daemonDir, locks); | ||
| while (locks.daemonLock.probe()) { | ||
| if (serverProcess != null && !serverProcess.isAlive()) { | ||
| System.err.println("Mill server exited!"); |
There was a problem hiding this comment.
I assume this is just debugging logging that should be cleaned up?
| sharedLoader = classOf[MillBuildBootstrap].getClassLoader, | ||
| sharedPrefixes = Seq("java.", "javax.", "scala.", "mill.api") | ||
| ) | ||
| val hasLayeredClassLoader = |
There was a problem hiding this comment.
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?
|
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 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 |
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:
org.scala-sbt:compiler-interfaceandorg.scala-sbt:test-interfacecore-apimodule of Mill (alongside its dependencies)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 inTestModule#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:
URLClassLoaders (and the JVM's app and bootstrap loaders), so that it's easier to inspect by users if they need toInitially, these development were motivated by suspicious things seen around
SystemStreams.ThreadLocalStreams.currentinvolving 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…