Make sure that evalaution runs with a proper classloader #473
Conversation
tgodzik
force-pushed
the
fix-classloader-issues
branch
2 times, most recently
from
a45a64a to
b0880e4
Compare
| reporter.error(e) | ||
| Document.empty(instrumentedInput) | ||
| val evaluatedDoc = new AtomicReference[Document]() | ||
| val thread = new Thread { |
There was a problem hiding this comment.
I packed it into a Thread and set the classloader we loaded the DocumentBuilder with. Not sure it's the best way to do it, but seems to work fine.
There was a problem hiding this comment.
I suggest using the option to hide whitespace changes,
There was a problem hiding this comment.
This is awesome that this is working. Great jump hunting this down. I get the changes, but maybe a silly question, but what class loader what then being used before this?
There was a problem hiding this comment.
It was the one we used for classloading mdoc I think, which didn't have the classes from dependencies.
tgodzik
force-pushed
the
fix-classloader-issues
branch
from
b0880e4 to
a5b6d74
Compare
|
@olafurpg does this make sense? I am always a bit unsure when it comes to classloaders etc. |
There was a problem hiding this comment.
I'm not exactly sure why this change fixes the problem. It would be nice (not necessary) if we could at least have a test case to reproduce the original so that we avoid future regression.
Implementation note: it would be nice to wrap the Thread logic into a helper method instead of having it all inline.
I don't think the AtomicReference is necessary, a var should work fine.
That's the Akka test case -> they try to load things from the current classlaoder, which is not the one containing Akka, so it's missing
Thanks! Good idea, I will try to improve it. |
tgodzik
force-pushed
the
fix-classloader-issues
branch
from
a5b6d74 to
f8bee43
Compare
Previously, we would run evalaution with the default mdoc classloader, which might have not contained things like default akka config. Now, we run it in a Thread with the specific classloader set. Fixes scalameta/metals#2525
tgodzik
force-pushed
the
fix-classloader-issues
branch
from
f8bee43 to
5dc9785
Compare
| @@ -92,6 +97,17 @@ object MarkdownBuilder { | |||
| new MarkdownCompiler(fullClasspath.syntax, scalacOptions) | |||
| } | |||
|
|
|||
| private def runInClassLoader(classloader: ClassLoader)(f: () => Unit) = { | |||
There was a problem hiding this comment.
Added a separate method for running in a classloader.
| """|import $dep.`com.typesafe.akka::akka-actor:2.6.13` | ||
| |import akka.actor.ActorSystem | ||
| | | ||
| |implicit val system = ActorSystem("worksheet") |
There was a problem hiding this comment.
I minimized the test case. Creating an actor system should pick up the default reference.conf from the classpath, which doesn't work without the changes.
Previously, we would run evalaution with the default mdoc classloader, which might have not contained things like default akka config. Now, we run it in a Thread with the specific classloader set.
Fixes scalameta/metals#2525