Aggressively clear the fields of the QuarkusClassLoader on close #41608
Conversation
quarkus-bot
bot
added
the
area/devtools
|
Seems reasonable, but we definitely want a +1 from @aloubyansky as well |
|
Yeah, that's why he's in the reviewer :). Also it's the first time I run a full CI with this patch so we might have some surprises :). |
Only good ones I hope :) |
|
Yeah, no, we already can see some failures. Probably because they try to load classes after the CL is closed, which is a problem I identified already. In most cases, it's not a big problem but from what I can see, in some cases it is... I'll have a look once I have a full picture and I will probably have to tone down things a bit, unfortunately :/ |
| @@ -131,7 +135,8 @@ public static boolean isResourcePresentAtRuntime(String resourcePath) { | |||
| PLATFORM_CLASS_LOADER = cl; | |||
| } | |||
|
|
|||
| private boolean closed; | |||
| private volatile boolean closing; | |||
| private volatile boolean closed; | |||
There was a problem hiding this comment.
A single variable representing a state will be better. I.e. in this case something like state with possible values OPEN, CLOSING, CLOSED, a type could be int or byte.
There was a problem hiding this comment.
Yeah, I was expecting that you would say that :).
| @@ -662,12 +713,19 @@ public void close() { | |||
| log.error("Failed to close " + element, e); | |||
| } | |||
| } | |||
| ResourceBundle.clearCache(this); | |||
|
|
|||
| classPathElements.clear(); | |||
There was a problem hiding this comment.
if we are doing that we could as well simply de-reference those instead of clearing
There was a problem hiding this comment.
Setting them to null would probably be problematic when we access the CL after it has been closed.
I'm not exactly optimistic on our capability to fix all the problematic cases soonish (if ever).
There was a problem hiding this comment.
you could use List.of() but i am not sure an empty list will actually work, we'll see what the CI says
|
In theory it sounds reasonable but it probably won't pass the CI yet |
This comment has been minimized.
This comment has been minimized.
|
I will try to classify the errors and see if we can improve the situation at least partially until we have a global fix. |
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. We also add some logging to debug the lifecycle of the class loader. We can't easily log things in the close() method so things are not as clean as they could be. That's the reason why we are using a system property to enable the logging. You can log the constructor and close() calls by enabling debug logging for category `io.quarkus.bootstrap.classloading.QuarkusClassLoader.lifecycle`. You can log late accesses to closed class loaders by passing `-Dquarkus-log-access-to-closed-class-loaders` to your build command.
If the class was already loaded, we can return it.
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.
We also add some logging to debug the lifecycle of the class loader. We can't easily log things in the close() method so things are not as clean as they could be. That's the reason why we are using a system property to enable the logging.
You can log the constructor and close() calls by enabling debug logging for category
io.quarkus.bootstrap.classloading.QuarkusClassLoader.lifecycle.You can log late accesses to closed class loaders by passing
-Dquarkus-log-access-to-closed-class-loadersto your build command.This work is related to the class loader leaks work I have been doing for weeks and also to these issues: