Netty finalizers load classes from closed class loaders #41607
Comments
|
/cc @cescoffier (netty), @franz1981 (netty), @jponge (netty) |
|
Note that this isn't the only issue I have with Netty. I also get the following for some tests: |
|
For the finalizer part I have sent a small fix - if you can give it a shot: https://github.com/franz1981/netty/tree/4.1_classloader_leak Ideally we would like to "just" rely on |
|
For the other one re scheduled tasks I think they are not cancelled, because netty/netty#9580 shows that we are currently removing them on cancel. |
|
@gsmet FYI netty/netty#14155 (comment) If is accepted - it means an additional property to us :"( |
|
ok @gsmet netty/netty#14155 is ready to go, so before is getting merged can you provide a feedback if it is working for your case? which disable finalizers for both jboss blocking thread pool and event loops (which should be both |
|
@franz1981 so the issue mentioned in the description looks solved by your PR. Still wondering if we could improve the unrelated situation I was mentioning in this comment: |
|
For that issue I have commented #41607 (comment) which means that we have some scheduled task which is stopped likely (just guessing, but I have no idea actually...) |
|
@cescoffier do you have any idea of what could schedule these tasks and what they could be? |
|
@franz1981 @cescoffier I added some breakpoints and AFAICS the tasks are from the Now it's hard to debug as things pop up everywhere but I wonder if it could be an interesting lead? |
|
It seems that https://github.com/netty/netty/blob/ef2ed4e2cda6f764c661e24caccc833f9fd0b6e5/common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java#L55 is a singleton event loop, single threaded, which, if has its single thread started, schedule a periodic task every each second to check if it should stop it, because idle. The executor itself should stop its single thread if there is nothing to do, see https://github.com/netty/netty/blob/ef2ed4e2cda6f764c661e24caccc833f9fd0b6e5/common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java#L278 (the |
|
This very naive patch seems to improve things: diff --git a/extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java b/extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java
index e0adae3bef1..30e8102f811 100644
--- a/extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java
+++ b/extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java
@@ -34,6 +34,7 @@
import io.netty.channel.EventLoopGroup;
import io.netty.util.concurrent.FastThreadLocal;
+import io.netty.util.concurrent.GlobalEventExecutor;
import io.quarkus.arc.Arc;
import io.quarkus.arc.InstanceHandle;
import io.quarkus.runtime.ExecutorRecorder;
@@ -453,6 +454,11 @@ public void handle(AsyncResult<Void> ar) {
}
LateBoundMDCProvider.setMDCProviderDelegate(null);
vertx = null;
+ try {
+ GlobalEventExecutor.INSTANCE.awaitInactivity(0, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ // ignore
+ }
}
}
Now, I think we would need a proper way to actually shutdown this executor. All the shutdown methods have been disabled for this class. |
|
The executor is not really "running": it lazily creates a single Thread to handle all tasks, which will stop after 1 second of inactivity (which is using a timer to detect it). While stopped it should be an empty shell, really. beware that if there is no At best this approach will force at least 1 second of pause between "restart": based on the configured |
|
I agree with Franz, we need to see why the executor is not shutdown nicely. I don't know if possible (didn't check the code yet) but in dev mode, we could imagine using reflection to shutdown the executor properly. Do you have a way to debug / trigger this @gsmet, I can try to have a look tomorrow or Monday. |
|
From what I can see, you have the As for reproducing it, I'm trying to get my PR in but the reviewing guys are fighting... :) |
|
@cescoffier @franz1981 so to reproduce it, make sure you have built a tree that contains: Then you can run:
You should see several stack traces similar to the following when the second test starts (you need two tests to see the issue): |
|
Coming back to this one. Will have a look this week. |
|
It seems that it's the following task that is running: final ScheduledFutureTask<Void> quietPeriodTask = new ScheduledFutureTask<Void>(
this, Executors.<Void>callable(new Runnable() {
@Override
public void run() {
// NOOP
}
}, null),
// note: the getCurrentTimeNanos() call here only works because this is a final class, otherwise the method
// could be overridden leading to unsafe initialization here!
deadlineNanos(getCurrentTimeNanos(), SCHEDULE_QUIET_PERIOD_INTERVAL),
-SCHEDULE_QUIET_PERIOD_INTERVAL
);This is the task that runs before closing the executor (I think @franz1981 knows better). |
@franz1981 @cescoffier I decided to create a specific issue for this one. I think it would have value for us to be able to fully control the Netty lifecycle instead of relying on finalizers.
Obviously, I have no idea how easy it would be.
This is the stacktrace we obtain:
This is logged when trying to load
IndexedQueueSizeUtilto check if the queue is empty.While it might sound like something that is not that problematic because it works, we actually have a lot of problems coming from the fact that we don't enforce the closing of CLs. When a
QuarkusClassLoaderis closed, we shouldn't try to load anything from it anymore.This is not exactly easy and this is just one of the occurrences but I thought it would be better isolated in a specific issue.
The text was updated successfully, but these errors were encountered: