LuciferYang commented on code in PR #12124:
URL: https://github.com/apache/gravitino/pull/12124#discussion_r3627557664
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -178,8 +184,25 @@ private static Thread[] getAllThreads() {
return threads;
}
- private static void clearThreadLocalMap(Thread thread, ClassLoader
targetClassLoader) {
- if (thread == null ||
!thread.getName().startsWith("Gravitino-webserver-")) {
+ @VisibleForTesting
+ static void clearThreadLocalMap(Thread thread, ClassLoader
targetClassLoader) {
+ if (thread == null) {
+ return;
+ }
+
+ // Sweep every application thread, not only the Gravitino-webserver-*
ones: a ThreadLocal
+ // pointing at the target ClassLoader can live on any thread, such as a
Caffeine ForkJoinPool
+ // worker, a catalog-cleaner thread, or a Hadoop daemon. The check below
clears only entries
+ // whose value was loaded by the target ClassLoader, so ThreadLocals owned
by other
+ // ClassLoaders are left alone.
+ //
+ // Skip threads whose immediate group is the JVM "system" group (Reference
Handler, Finalizer,
+ // Signal Dispatcher, and the like): they hold no catalog ClassLoader
references, and reflecting
+ // into their ThreadLocals is best avoided. This checks the immediate
group only, so threads in
+ // a sub-group of system (such as InnocuousThreadGroup for common-pool
workers) are still swept.
+ // That is intended: ForkJoinPool threads can hold catalog ThreadLocals.
+ ThreadGroup group = thread.getThreadGroup();
+ if (group != null && "system".equals(group.getName())) {
Review Comment:
Good question, I verified it with a test rather than reasoning about it.
Added `testClearThreadLocalMapLetsDroppedClassLoaderBeCollected`: a
long-lived, non-`Gravitino-webserver-*` worker thread holds a `ThreadLocal`
whose value is loaded by the target ClassLoader, which is what pins a dropped
catalog's ClassLoader in production. After `clearThreadLocalMap` runs and the
test drops its own references, a `WeakReference` to the ClassLoader clears once
GC runs. Under the old `Gravitino-webserver-` name filter this exact test fails
(the thread is skipped, the ClassLoader stays reachable), so it both proves the
fix and guards against a regression.
Two things worth clarifying about the scope of the change:
- It only changes *which threads' ThreadLocals are swept*, not *which
threads are stopped*. The interrupt path in
`stopThreadsAndClearThreadLocalVariables` is still guarded by
`runningWithClassLoader` (contextClassLoader == target), unchanged. And an
entry is only cleared when `value.getClass().getClassLoader() ==
targetClassLoader`, so ThreadLocals owned by other ClassLoaders are never
touched. The worst case of the wider scope is visiting a thread and skipping it.
- The reflective `ThreadLocalMap` access needs `--add-opens
java.base/java.lang=ALL-UNNAMED`, which `bin/gravitino.sh.template` already
sets, so it works at runtime.
`./gradlew :catalogs:catalog-common:test --tests
"*TestClassLoaderResourceCleanerUtils"`: 8/8 green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
