The big ClassLoader change #6411
Conversation
stuartwdouglas
force-pushed
the
new-class-loading2
branch
4 times, most recently
from
3e02fa7 to
48c6c20
Compare
core/deployment/src/main/java/io/quarkus/runner/bootstrap/GenerateConfigTask.java
Outdated
Show resolved
Hide resolved
core/deployment/src/main/java/io/quarkus/runner/bootstrap/GenerateConfigTask.java
Outdated
Show resolved
Hide resolved
test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
Outdated
Show resolved
Hide resolved
stuartwdouglas
force-pushed
the
new-class-loading2
branch
2 times, most recently
from
5cab6bb to
956aa03
Compare
|
This commit includes a class loading guide that gives a bit of an explanation as to how it all works, when reviewing this is probably a good place to start. |
stuartwdouglas
force-pushed
the
new-class-loading2
branch
from
a178bac to
296a2b6
Compare
| This loads all the `-deployment` artifacts and their dependencies, as well as other user dependencies. It does not load the | ||
| application root or any hot deployed code. This ClassLoader is persistent, even if the application restarts it will remain | ||
| (which is why it cannot load application classes that may be hot deployed). It's parent is the base ClassLoader, and it is | ||
| transformer safe. |
There was a problem hiding this comment.
I think we should have an explanation of what that means.
There was a problem hiding this comment.
The explanation exists, but it's further down, so I agree it should be moved up here
There was a problem hiding this comment.
Can you also please add an explanation why this classloader needs to be transformer safe? Is this need driven by the fact that we need to transform dependencies in some cases?
|
|
||
| This ClassLoader is non-persistent, it will be re-created when the application is started, and is isolated. This ClassLoader | ||
| is the context ClassLoader that is used when running the build steps. It is transformer safe so loading application classes | ||
| will not prevent them from being transformed. |
| There are some dependencies that we can be sure we do not want. This generally happens when a dependency has had a name | ||
| change (e.g. smallrye-config changing groups from `org.smallrye` to `org.smallrye.config`, the `javax` -> `jakarta` rename). | ||
| This can cause problems, as if these artifacts end up in the dependency tree out of date classes can be loaded that are | ||
| not compatible with Quarkus. To deal with this this extensions can specify artifacts that should never be loaded. This is |
There was a problem hiding this comment.
| not compatible with Quarkus. To deal with this this extensions can specify artifacts that should never be loaded. This is | |
| not compatible with Quarkus. To deal with this, extensions can specify artifacts that should never be loaded. This is |
|
|
||
| Base Runtime ClassLoader:: | ||
|
|
||
| This loads all the runtime extension dependencies, as well as other user dependencies. It does not load the |
There was a problem hiding this comment.
as well as other user dependencies is mentioned here for Base Runtime Classloader, however Augment ClassLoader also mentions as well as other user dependencies.
This seems kind of confusing, but I assume that the former is done at build time and the latter at runtime? Maybe a note should be made about this?
|
|
||
| This loads code that is not hot-reloadable, but it does support transformation (although once the class is loaded this | ||
| transformation is no longer possible). This means that only transformers registered in the first application start | ||
| will take effect, however as these transformers are expected to be idempotent this should not cause problems. |
There was a problem hiding this comment.
Could we have an example of what sort of transformations are meant here?
|
I took at this a little and I really love the way things get a lot cleaner |
stuartwdouglas
force-pushed
the
new-class-loading2
branch
7 times, most recently
from
05ca120 to
47ae5f5
Compare
...ests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java
Outdated
Show resolved
Hide resolved
|
The windows tests are failing because something somewhere is not closing a file handle for the libs/$app}.jar file in the gradle build. The gradle tests can't delete their temp folders and fail as a result. |
stuartwdouglas
force-pushed
the
new-class-loading2
branch
2 times, most recently
from
6ab8421 to
92f80fe
Compare
boring-cyborg
bot
added
area/core
area/dependencies
stuartwdouglas
force-pushed
the
new-class-loading2
branch
2 times, most recently
from
c17a597 to
cab3589
Compare
|
@aloubyansky I believe I have fixed all the issues you brought up. @gsmet I have fixed the docs issues. I have tested against the platform test suite, and there are some issues that will likely need to be fixed on the camel side (e.g. the XML parsing issue I mention above). Once this is in I will work with them to get this resolved. |
stuartwdouglas
force-pushed
the
new-class-loading2
branch
from
cab3589 to
f4aa3ff
Compare
| Files.createDirectories(configFile.getParent()); | ||
| Files.write(configFile, sb.toString().getBytes(StandardCharsets.UTF_8), | ||
| Files.exists(configFile) ? new OpenOption[] { StandardOpenOption.APPEND } : new OpenOption[] {}); | ||
| Files.deleteIfExists(temp); |
There was a problem hiding this comment.
Shouldn't it be deleted in the finally?
| classWriter.write(i.getClassData()); | ||
| } | ||
| log.infof("Wrote %s", classFile.getAbsolutePath()); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
Does it really have to be a Throwable? Swallowing throwables in a loop may go pretty bad.
stuartwdouglas
force-pushed
the
new-class-loading2
branch
3 times, most recently
from
1553a1d to
ee88497
Compare
stuartwdouglas
force-pushed
the
new-class-loading2
branch
4 times, most recently
from
751414c to
a0d6e82
Compare
|
Damn CI is acting up again... |
|
I made another modification today as I found a class loader leak, if this passed it should be good to merge |
stuartwdouglas
force-pushed
the
new-class-loading2
branch
from
a0d6e82 to
9b73548
Compare
|
CI seems to be working again, @gsmet if it passes can you hit the button? |
gsmet
added
the
triage/waiting-for-ci
|
Sure. |
This changes the way Quarkus ClassLoading works, to allow for isolated class loaders. It also unifies how Quarkus is launched, so every different mode we support uses the same mechanism for both curation and launch. Tests are now run in an isolated ClassLoader, which means that a proxy is created that runs the tests from within the isolated ClassLoader. This currently has a quirk where @BeforeAll methods are run twice, which will be fixed in the next JUnit release. This can be worked around by using @QuarkusBeforeAll.
gsmet
force-pushed
the
new-class-loading2
branch
from
9b73548 to
b67491c
Compare
|
I rebased to trigger a new CI run... This CI issue is a nightmare :/ |
Fixes #5597 #1724 #5354 #770 #3917 #6652
This changes the way Quarkus ClassLoading works,
to allow for isolated class loaders.
It also unifies how Quarkus is launched, so every
different mode we support uses the same mechanism
for both curation and launch.
Tests are now run in an isolated ClassLoader, which
means that a proxy is created that runs the tests
from within the isolated ClassLoader. This currently
has a quirk where @BeforeAll methods are run twice,
which will be fixed in the next JUnit release. This
can be worked around by using @QuarkusBeforeAll.
Apparently JUnit 5.6 which will fix this should be released on the 20th according to https://github.com/junit-team/junit5/milestone/46, which will allow this restriction to be removed.
This still needs performance testing, but otherwise is very close to being complete.
All of these should be addressed in the new few days.