Attempt to alleviate QuarkusClassLoader leaks #41172
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
quarkus-bot
bot
added
the
area/devtools
This comment has been minimized.
This comment has been minimized.
|
|
quarkus-bot
bot
added
area/jbang
- The TestResourceManager is instantiated using the runtime class loader so we need to make sure we close it before we close the runtime class loader. Otherwise we can get into trouble if we need to load some additional classes. - Also make sure that it is only closed once. It used to be closed once when the application is stopped and then again when the QuarkusTestExtensionState was closed.
We know that the class loaders can leak, for instance due to long lived resources that span the boundaries of a test so we try to limit the effects by clearing the fields from the class loader and especially all the ClassPathElements. Note that for now, I didn't nullify the parent field that points to the parent CL but I wonder if we should do it.
When closing a ZipFileSystem, there is a non trivial amount of data that is still kept around and in particular the cen field.
Not entirely sure we will keep it but let's run the test suite with it.
Currently, we pass the CL to something that will close it, rather than controlling the lifecycle in the creator method. This is an issue in native ITs: java.lang.RuntimeException: java.lang.IllegalStateException: This class loader has been closed at io.quarkus.test.junit.QuarkusIntegrationTestExtension.throwBootFailureException(QuarkusIntegrationTestExtension.java:373) at io.quarkus.test.junit.QuarkusIntegrationTestExtension.beforeEach(QuarkusIntegrationTestExtension.java:117) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) Caused by: java.lang.IllegalStateException: This class loader has been closed at io.quarkus.bootstrap.classloading.QuarkusClassLoader.ensureOpen(QuarkusClassLoader.java:716) at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:495) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:467) at io.quarkus.runner.bootstrap.AugmentActionImpl.performCustomBuild(AugmentActionImpl.java:158) at io.quarkus.test.junit.IntegrationTestUtil.handleDevServices(IntegrationTestUtil.java:297) at io.quarkus.test.junit.QuarkusIntegrationTestExtension.doProcessStart(QuarkusIntegrationTestExtension.java:199) at io.quarkus.test.junit.QuarkusIntegrationTestExtension.ensureStarted(QuarkusIntegrationTestExtension.java:169) at io.quarkus.test.junit.QuarkusIntegrationTestExtension.beforeAll(QuarkusIntegrationTestExtension.java:130)
The Augmentation CL is the parent of the deployment CL, so the deployment CL must be closed first. This is consistent with what happens when we don't do it manually.
That way, it's easier to differentiate the CL, their lifecycle and what they are used for.
And also avoid passing the CuratedApplication here. The problem was noticed when trying to log something in the QuarkusClassLoader constructor.
When popping provider factory, what we actually want is pop it if something has been pushed. It is better handled by setting a boolean if something has been pushed. This allows class loading issues when the class loader has been closed and the request is still processing.
There's no real need here and it tries to load classes after the CL has been closed.
|
I have extracted all the work done here (and more!) in small PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This won't fix the leaks as there are multiple reasons why CLs are leaking in tests and dev mode but... I thought we should consider at least reducing the consequences of the leaks by clearing the resources and making sure that the
ZipFileSystemcan be garbage collected, together with some other elements.Now there's something a bit odd, if I set
statetonull, I have a very weird error:Which means that somehow the closed class loader is still in use - which is extremely weird... If someone has an idea, I'm interested.
Commit messages have a bit more details.
Related to #41158 and #41156 .