yuqi1129 commented on code in PR #12124:
URL: https://github.com/apache/gravitino/pull/12124#discussion_r3931970420
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -355,6 +378,36 @@ static boolean isOwnedByClassLoader(Class<?> clazz,
ClassLoader classLoader) {
return classLoader != null && clazz.getClassLoader() == classLoader;
}
+ /**
+ * Shutdown MySQL's AbandonedConnectionCleanupThread to prevent it from
holding a reference to the
+ * ClassLoader. Unlike simple thread interruption, {@code
uncheckedShutdown()} cleans up the
+ * tracked connections map, releasing references to classes loaded by the
target ClassLoader.
+ *
+ * @param classLoader the classloader where the MySQL driver is loaded
+ */
+ @VisibleForTesting
+ static void shutdownMySQLAbandonedConnectionCleanupThread(ClassLoader
classLoader)
+ throws Exception {
+ // Load with initialize=false: when this class is owned by the target
ClassLoader the MySQL
+ // driver has already been used, so the class is initialized and its
cleanup thread started.
+ Class<?> clazz =
+ Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread",
false, classLoader);
+ // uncheckedShutdown() is a JVM-global static that stops the single
cleanup thread tied to this
+ // driver class. If the driver was resolved from a parent or shared
ClassLoader (for example a
+ // MySQL entity store loaded on the app ClassLoader), stopping it here
would break
+ // abandoned-connection cleanup for the whole JVM. Only act when the
target ClassLoader owns the
+ // class, the same guard the other cleanup steps in this class use.
+ if (!isOwnedByClassLoader(clazz, classLoader)) {
+ LOG.debug(
+ "AbandonedConnectionCleanupThread is owned by {}, not the target
classloader {}; skipping shutdown",
+ clazz.getClassLoader(),
+ classLoader);
+ return;
+ }
+ MethodUtils.invokeStaticMethod(clazz, "uncheckedShutdown");
Review Comment:
I verified the mechanism this is built on, and it holds. Notes for whoever
reviews next.
`mysql-connector-j` 8.0.33 (the version pinned in `libs.versions.toml`)
creates the cleanup thread through a factory that explicitly pins the loader:
```
16: ldc class com/mysql/cj/jdbc/AbandonedConnectionCleanupThread
18: invokevirtual Class.getClassLoader()
32: invokevirtual Thread.setContextClassLoader(ClassLoader)
37: putstatic Field threadRef:Ljava/lang/Thread;
```
So a live cleanup thread pins whichever loader defined the driver class, and
the static `threadRef` keeps the thread reachable. Note it is `implements
Runnable`, not a `Thread` subclass, so the pin is through the Runnable and the
TCCL rather than the thread's own class.
I also ran an independent reclaim probe with a control group:
```
[CONTROL] child loader that never touches MySQL -> collected: true
[NO FIX] load the driver class, drop the loader -> collected: false
[WITH FIX] same, then uncheckedShutdown() -> collected: true
```
The control matters: without it, `collected: false` could just mean the
WeakReference test was written badly. With it, the leak and the fix are both
real.
The `isOwnedByClassLoader` guard here is also correct, and I want to say
that explicitly so nobody "fixes" it later: two isolated loaders each holding
their own driver copy have separate classes and separate statics, so there is
no cross-talk; a shared or parent-loaded driver is skipped; and
`ClassLoaderPool` only calls this at refcount zero.
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -77,6 +79,10 @@ public static void closeClassLoaderResource(ClassLoader
classLoader) {
executeAndCatch(ClassLoaderResourceCleanerUtils::closeResourceInAzure,
classLoader);
executeAndCatch(ClassLoaderResourceCleanerUtils::clearShutdownHooks,
classLoader);
+
+ executeAndCatch(
+
ClassLoaderResourceCleanerUtils::shutdownMySQLAbandonedConnectionCleanupThread,
Review Comment:
**Ordering looks wrong.** `runningWithClassLoader` matches on
`thread.getContextClassLoader() == targetClassLoader`, and the bytecode above
shows the MySQL cleanup thread's TCCL is set to exactly the loader that defined
the driver, i.e. the target loader.
So by the time this new step runs, `stopThreadsAndClearThreadLocalVariables`
(six steps earlier) has already matched that thread and done
`setContextClassLoader(null)` + `interrupt()` on it. MySQL's `run()` blocks in
`referenceQueue.remove()`, so it gets an out-of-band `InterruptedException`
instead of the driver's own shutdown path, and whether
`connectionFinalizerPhantomRefs` is left populated at that point is not
something the driver promises.
The end state is probably still correct, because `uncheckedShutdown()`
clears the map afterwards, but that is the second step cleaning up after the
first, which is fragile.
Worth being explicit about the limits of my verification: my probe called
`shutdownMySQLAbandonedConnectionCleanupThread` **directly**, not through
`closeClassLoaderResource`, so I confirmed the method works but did **not**
exercise this ordering.
Minimal fix: move this `executeAndCatch` above the
`stopThreadsAndClearThreadLocalVariables` call. The driver then exits through
its own API and the generic sweep finds no thread to interrupt.
##########
catalogs/catalog-common/src/test/java/org/apache/gravitino/utils/TestClassLoaderResourceCleanerUtils.java:
##########
@@ -68,4 +78,288 @@ void
testIsOwnedByClassLoaderReturnsFalseForBootstrapLoadedClass() {
ClassLoaderResourceCleanerUtils.isOwnedByClassLoader(
String.class, ClassLoader.getSystemClassLoader()));
}
+
+ /**
+ * Loading MySQL's {@code AbandonedConnectionCleanupThread} starts a daemon
thread {@code
+ * mysql-cj-abandoned-connection-cleanup} whose contextClassLoader is the
loader that loaded it.
+ * That reference pins the ClassLoader and leaks Metaspace after a catalog
is dropped. {@code
+ * shutdownMySQLAbandonedConnectionCleanupThread} must terminate that thread
when the class is
+ * owned by the target loader.
+ */
+ @Test
+ void testShutdownMySQLAbandonedConnectionCleanupThreadStopsThread() throws
Exception {
+ URL mysqlJar = mysqlConnectorJarUrl();
+ Assumptions.assumeTrue(mysqlJar != null, "mysql-connector jar is not on
the test classpath");
+
+ // Parent is the platform loader so `child` loads its OWN copy of the
class (no delegation to
+ // the app loader): the cleanup thread's contextClassLoader is `child` and
the ownership guard
+ // passes. Load AbandonedConnectionCleanupThread directly rather than the
Driver so the thread
+ // starts without registering a child-loaded Driver into the JVM-global
DriverManager. That
+ // registration would pin `child` after this test and leak it, which is
the leak this utility
+ // exists to prevent.
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+ try (URLClassLoader child = new URLClassLoader(new URL[] {mysqlJar},
platform)) {
+ Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread",
true, child);
+ assertNotNull(
+ findCleanupThreadBoundTo(child),
+ "loading AbandonedConnectionCleanupThread should start the cleanup
thread bound to child");
+
+
ClassLoaderResourceCleanerUtils.shutdownMySQLAbandonedConnectionCleanupThread(child);
+
+ assertTrue(
+ waitForCleanupThreadGone(child),
+ "cleanup thread bound to the child loader should be gone after
uncheckedShutdown");
+ }
+ }
+
+ /**
+ * When the MySQL class resolves to a parent/shared ClassLoader (the driver
sits on a shared
+ * classpath while the catalog being closed has its own child loader), {@code
+ * shutdownMySQLAbandonedConnectionCleanupThread} must skip the JVM-global
{@code
+ * uncheckedShutdown()} so closing one catalog does not stop a cleanup
thread owned by another
+ * loader.
+ */
+ @Test
+ void
testShutdownMySQLAbandonedConnectionCleanupThreadSkipsParentOwnedClass() throws
Exception {
+ URL mysqlJar = mysqlConnectorJarUrl();
+ Assumptions.assumeTrue(mysqlJar != null, "mysql-connector jar is not on
the test classpath");
+
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+ // `owner` holds the class; `child` delegates to it, mirroring a driver on
a shared parent CL.
+ try (URLClassLoader owner = new URLClassLoader(new URL[] {mysqlJar},
platform);
+ URLClassLoader child = new URLClassLoader(new URL[0], owner)) {
+ Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread",
true, owner);
+ assertNotNull(
+ findCleanupThreadBoundTo(owner), "cleanup thread should be bound to
the owner loader");
+
+ // classLoader=child, but the class resolves to `owner` via delegation,
so the guard skips.
+
ClassLoaderResourceCleanerUtils.shutdownMySQLAbandonedConnectionCleanupThread(child);
+ // When uncheckedShutdown() does run it kills the thread within ~100ms
(see the positive
+ // test's poll). Wait past that window and confirm the owner's thread is
still alive, which
+ // shows the guard skipped it rather than just not having stopped it
yet. Without the guard,
+ // 1s is long enough for the shutdown to take effect and this assertion
fails.
+ assertTrue(
+ cleanupThreadStaysAliveFor(owner, 1000),
+ "cleanup thread owned by the parent loader must survive a
child-scoped cleanup");
+
+ // An owner-scoped cleanup does run (guard passes), so nothing lingers
past this test.
+
ClassLoaderResourceCleanerUtils.shutdownMySQLAbandonedConnectionCleanupThread(owner);
+ assertTrue(
+ waitForCleanupThreadGone(owner), "owner-scoped cleanup should stop
the thread it owns");
+ }
+ }
+
+ /**
+ * The broadened {@code clearThreadLocalMap} must clear a ThreadLocal whose
value is owned by the
+ * target ClassLoader even on a non-{@code Gravitino-webserver-*} thread
(the pre-#10093 code only
+ * touched webserver threads). A ThreadLocal holding a value from a
different ClassLoader must be
+ * left untouched, which is the {@code isOwnedByClassLoader}-style guard
inside the sweep.
+ */
+ @Test
+ @SuppressWarnings("ThreadLocalUsage") // local ThreadLocals are required to
test per-thread sweep
+ void testClearThreadLocalMapClearsOnlyTargetClassLoaderValues() throws
Exception {
+ URL classesUrl =
LeakyValue.class.getProtectionDomain().getCodeSource().getLocation();
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+ ThreadLocal<Object> targetOwned = new ThreadLocal<>();
+ ThreadLocal<Object> foreign = new ThreadLocal<>();
+ // `child` is the cleanup target; `sibling` is an unrelated loader. Both
load their own copy of
+ // LeakyValue, so both values have a non-null ClassLoader. This exercises
the real
+ // discrimination (value CL == target vs a different non-null CL), not
just the null-CL case.
+ try (URLClassLoader child = new URLClassLoader(new URL[] {classesUrl},
platform);
+ URLClassLoader sibling = new URLClassLoader(new URL[] {classesUrl},
platform)) {
+ Object childOwnedValue =
+ Class.forName(LeakyValue.class.getName(), true, child)
+ .getDeclaredConstructor()
+ .newInstance();
+ assertSame(child, childOwnedValue.getClass().getClassLoader());
+ Object siblingOwnedValue =
+ Class.forName(LeakyValue.class.getName(), true, sibling)
+ .getDeclaredConstructor()
+ .newInstance();
+ assertSame(sibling, siblingOwnedValue.getClass().getClassLoader());
+
+ // This test runs on the JUnit "main"/worker thread, deliberately NOT a
webserver thread, so
+ // it also proves the broadened scope.
+
assertFalse(Thread.currentThread().getName().startsWith("Gravitino-webserver-"));
+
+ targetOwned.set(childOwnedValue);
+ foreign.set(siblingOwnedValue);
+
+
ClassLoaderResourceCleanerUtils.clearThreadLocalMap(Thread.currentThread(),
child);
+
+ assertNull(targetOwned.get(), "ThreadLocal owned by the target
ClassLoader must be cleared");
+ assertSame(
+ siblingOwnedValue,
+ foreign.get(),
+ "ThreadLocal owned by a different non-null ClassLoader must be
kept");
+ } finally {
+ targetOwned.remove();
+ foreign.remove();
+ }
+ }
+
+ /**
+ * JVM-internal threads live in the {@code system} thread group; the sweep
must skip them entirely
+ * rather than reflect into their ThreadLocals.
+ */
+ @Test
+ @SuppressWarnings("ThreadLocalUsage") // local ThreadLocal is required to
test per-thread sweep
+ void testClearThreadLocalMapSkipsSystemThreadGroup() throws Exception {
+ ClassLoader loader =
ClassLoaderResourceCleanerUtils.class.getClassLoader();
+ ThreadGroup systemGroup = systemThreadGroup();
+ assertNotNull(systemGroup, "expected to locate the system thread group");
+
+ Object[] cleared = new Object[1];
+ Thread systemThread =
+ new Thread(
+ systemGroup,
+ () -> {
+ ThreadLocal<Object> tl = new ThreadLocal<>();
+ tl.set(new LeakyValue());
+ // Sweeping a system-group thread must be a no-op: the value
stays set.
+
ClassLoaderResourceCleanerUtils.clearThreadLocalMap(Thread.currentThread(),
loader);
+ cleared[0] = tl.get();
+ },
+ "clp-system-group-probe");
+ systemThread.start();
+ systemThread.join();
+
+ assertNotNull(cleared[0], "system-group thread ThreadLocals must be left
untouched");
+ }
+
+ /**
+ * End-to-end proof that broadening the sweep lets a dropped catalog's
ClassLoader be collected. A
+ * long-lived worker thread that is not a {@code Gravitino-webserver-*}
thread holds a ThreadLocal
+ * whose value was loaded by the target ClassLoader, which is exactly what
pins the ClassLoader
+ * after a catalog is dropped. After {@code clearThreadLocalMap} runs and
the test drops its own
+ * references, a WeakReference to the ClassLoader must clear once GC runs.
The webserver-only
+ * filter used before would skip this thread and the ClassLoader would stay
reachable.
+ */
+ @Test
+ @SuppressWarnings("ThreadLocalUsage") // a per-thread ThreadLocal is the
leak this test reproduces
+ void testClearThreadLocalMapLetsDroppedClassLoaderBeCollected() throws
Exception {
Review Comment:
This is a good test and it covers the ThreadLocal half of the issue end to
end.
The MySQL half has no equivalent:
`testShutdownMySQLAbandonedConnectionCleanupThreadStopsThread` asserts the
thread is gone, but not that the ClassLoader then becomes collectable, which is
the property the issue is actually about. I wrote that test as a throwaway
while verifying this PR and it passes, so it is worth having here:
```
[CONTROL] child loader that never touches MySQL -> collected: true
[NO FIX] load the driver class, drop the loader -> collected: false
[WITH FIX] same, then uncheckedShutdown() -> collected: true
```
The control case is the part worth copying — a bare `WeakReference` +
`System.gc()` assertion is unreliable enough that without it a green test does
not mean much.
##########
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:
This widens a genuinely unsafe operation. `clearThreadLocalMap` reflects
into another thread's `ThreadLocalMap.table` and writes `entry.value = null`
from a foreign thread with no synchronisation. Until now that was limited to
`Gravitino-webserver-*`, which are mostly idle between requests. Skipping only
the immediate `system` group means we now also sweep `InnocuousThreadGroup`
(ForkJoinPool common-pool workers) and Hadoop daemons, and the comment says
that is deliberate — those are threads actively running work.
Structural corruption is not the risk (rehash moves `Entry` references, so
writing `value` still lands on the right object). The risk is nulling a value
another thread is using right now, producing an NPE in unrelated code at
catalog-drop time, which is close to impossible to attribute afterwards.
One thing that makes me more comfortable with the change than I expected: I
checked the blame, and the `Gravitino-webserver-` restriction has been there
since the class was introduced in #8252, so this is extending an incomplete fix
rather than reversing a deliberate safety decision. Worth saying so in the PR
description.
Suggestion: keep the widening but bound it — an allowlist of the thread
names actually implicated (`Gravitino-webserver-*`, `ForkJoinPool*`,
`catalog-cleaner*`, Hadoop's statistics cleaner) reads better than "everything
except the system group", and it documents what we are actually chasing.
Skipping threads currently in `RUNNABLE` would be another way.
##########
catalogs/catalog-common/src/test/java/org/apache/gravitino/utils/TestClassLoaderResourceCleanerUtils.java:
##########
@@ -20,17 +20,27 @@
package org.apache.gravitino.utils;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.lang.ref.WeakReference;
import java.net.URL;
import java.net.URLClassLoader;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Test;
class TestClassLoaderResourceCleanerUtils {
Review Comment:
Test-structure point: every case here calls
`shutdownMySQLAbandonedConnectionCleanupThread` or `clearThreadLocalMap`
directly, so nothing exercises `closeClassLoaderResource` itself. That is why
the step-ordering problem I flagged on the main file is invisible to this suite.
One integration case through `closeClassLoaderResource` would cover it. Note
it early-returns when the `GRAVITINO_TEST` environment variable is set, so such
a test has to clear that first.
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -355,6 +378,36 @@ static boolean isOwnedByClassLoader(Class<?> clazz,
ClassLoader classLoader) {
return classLoader != null && clazz.getClassLoader() == classLoader;
}
+ /**
+ * Shutdown MySQL's AbandonedConnectionCleanupThread to prevent it from
holding a reference to the
+ * ClassLoader. Unlike simple thread interruption, {@code
uncheckedShutdown()} cleans up the
+ * tracked connections map, releasing references to classes loaded by the
target ClassLoader.
+ *
+ * @param classLoader the classloader where the MySQL driver is loaded
+ */
+ @VisibleForTesting
+ static void shutdownMySQLAbandonedConnectionCleanupThread(ClassLoader
classLoader)
Review Comment:
Minor, maintainability. This class is accumulating vendor special cases
(AWS, GCP, Azure, commons-logging, now MySQL), and the same shape exists for
other JDBC drivers we ship catalogs for.
No need to abstract anything now, but a line in the class javadoc recording
the pattern would help the next person: guard with `isOwnedByClassLoader`, use
the driver's own shutdown API rather than an interrupt, and run it **before**
the generic thread sweep.
--
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]
