Copilot commented on code in PR #12124:
URL: https://github.com/apache/gravitino/pull/12124#discussion_r3957130628
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -355,6 +408,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:
`shutdownMySQLAbandonedConnectionCleanupThread` will throw (and likely log
via `executeAndCatch`) on catalogs that don’t have MySQL on the target
ClassLoader (ClassNotFoundException) and may also fail on driver versions that
don’t expose `uncheckedShutdown`. Since this cleanup sequence is run
unconditionally, this can generate noisy logs and potentially mask real cleanup
failures. Consider handling `ClassNotFoundException` as an expected no-op
(debug-level) and being defensive about the shutdown API (e.g., try
`uncheckedShutdown`, otherwise fall back to the older `shutdown` method if
present, otherwise skip with a debug log).
##########
catalogs/catalog-common/src/test/java/org/apache/gravitino/utils/TestClassLoaderResourceCleanerUtils.java:
##########
@@ -68,4 +78,365 @@ 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 {
+ URL classesUrl =
LeakyValue.class.getProtectionDomain().getCodeSource().getLocation();
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+
+ WeakReference<ClassLoader> ref;
+ // Blocks the worker after it sets the ThreadLocal so the thread stays
alive; a thread that
+ // exits would null its own threadLocals in Thread.exit() and hide the
leak.
+ CountDownLatch release = new CountDownLatch(1);
+ Thread worker = null;
+ try {
+ URLClassLoader child = new URLClassLoader(new URL[] {classesUrl},
platform);
+ Object childOwnedValue =
+ Class.forName(LeakyValue.class.getName(), true, child)
+ .getDeclaredConstructor()
+ .newInstance();
+ // Guard the test's premise: the value must be loaded by `child`,
otherwise the sweep's
+ // value-ClassLoader check never matches and the reclamation below would
prove nothing.
+ assertSame(child, childOwnedValue.getClass().getClassLoader());
+ ref = new WeakReference<>(child);
+
+ CountDownLatch valueSet = new CountDownLatch(1);
+ Object[] valueHolder = {childOwnedValue};
+ worker =
+ new Thread(
+ () -> {
+ ThreadLocal<Object> tl = new ThreadLocal<>();
+ tl.set(valueHolder[0]);
+ valueSet.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException ignored) {
+ // test teardown
+ }
+ },
+ "clp-reclaim-probe");
+ worker.setDaemon(true);
+ worker.start();
+ assertTrue(valueSet.await(10, TimeUnit.SECONDS), "worker should set its
ThreadLocal");
+
+ ClassLoaderResourceCleanerUtils.clearThreadLocalMap(worker, child);
+
+ // Drop every strong reference the test holds; the only thing that could
still pin `child` is
+ // the worker's ThreadLocal, which the sweep just cleared.
+ child.close();
+ child = null;
+ childOwnedValue = null;
+ valueHolder[0] = null;
+
+ // Assert before releasing the worker: once it returns, Thread.exit()
nulls its threadLocals
+ // and would let `child` be collected even if the sweep did nothing.
+ assertTrue(
+ awaitCollected(ref),
+ "dropped ClassLoader must be collectable once its ThreadLocal is
swept");
+ } finally {
+ release.countDown();
+ if (worker != null) {
+ worker.join(1000);
+ }
+ }
+ }
+
+ /**
+ * The property the issue is actually about: stopping the cleanup thread has
to make the dropped
+ * ClassLoader collectable, not merely make the thread go away. The control
case matters, because
+ * a bare WeakReference plus System.gc() assertion is unreliable enough that
a green result
+ * without one proves little.
+ */
+ @Test
+ void
testShutdownMySQLAbandonedConnectionCleanupThreadLetsDroppedClassLoaderBeCollected()
+ throws Exception {
+ URL mysqlJar = mysqlConnectorJarUrl();
+ Assumptions.assumeTrue(mysqlJar != null, "mysql-connector jar is not on
the test classpath");
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+
+ // Control: the same shape of child loader, never asked for the driver. If
this one were not
+ // collectable either, the assertions below would be measuring the probe,
not the leak.
+ URLClassLoader control = new URLClassLoader(new URL[] {mysqlJar},
platform);
+ WeakReference<ClassLoader> controlRef = new WeakReference<>(control);
+ control.close();
+ control = null;
+ assertTrue(
+ awaitCollected(controlRef),
+ "a child loader that never loaded the driver must be collectable");
+
+ URLClassLoader child = new URLClassLoader(new URL[] {mysqlJar}, platform);
+ Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread", true,
child);
+ assertNotNull(findCleanupThreadBoundTo(child), "the cleanup thread should
be running");
+ WeakReference<ClassLoader> ref = new WeakReference<>(child);
+ child.close();
+ child = null;
+
+ // The leak: the running cleanup thread pins the loader through its
context ClassLoader.
+ assertFalse(
+ awaitCollected(ref), "a live cleanup thread must keep the dropped
loader reachable");
+
+ // Still reachable, so the test can get the loader back to run the fix on
it.
+ ClassLoader leaked = ref.get();
+ assertNotNull(leaked, "loader should still be reachable while its cleanup
thread runs");
+
ClassLoaderResourceCleanerUtils.shutdownMySQLAbandonedConnectionCleanupThread(leaked);
+ assertTrue(
+ waitForCleanupThreadGone(leaked), "cleanup thread should be gone after
the shutdown");
+ leaked = null;
+
+ assertTrue(
+ awaitCollected(ref), "loader must be collectable once its cleanup
thread is stopped");
+ }
+
+ /**
+ * Drives the whole cleanup sequence rather than one step. Every other case
here calls a single
+ * step directly, so nothing covers the sequence, which is where step
ordering lives. This asserts
+ * the sequence's outcome; it cannot by itself distinguish one order from
another, since a later
+ * step can cover for an earlier one.
+ *
+ * <p>Goes through {@code runCleanupSteps} rather than {@code
closeClassLoaderResource}: the
+ * latter returns immediately when {@code GRAVITINO_TEST} is set, which the
build sets for every
+ * test task.
+ */
+ @Test
+ void testRunCleanupStepsStopsTheDriverThreadAndFreesTheClassLoader() throws
Exception {
+ URL mysqlJar = mysqlConnectorJarUrl();
+ Assumptions.assumeTrue(mysqlJar != null, "mysql-connector jar is not on
the test classpath");
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+
+ URLClassLoader child = new URLClassLoader(new URL[] {mysqlJar}, platform);
+ Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread", true,
child);
+ assertNotNull(findCleanupThreadBoundTo(child), "the cleanup thread should
be running");
+ WeakReference<ClassLoader> ref = new WeakReference<>(child);
+
+ ClassLoaderResourceCleanerUtils.runCleanupSteps(child);
+
+ assertTrue(
+ waitForCleanupThreadGone(child),
+ "the sequence should leave no cleanup thread on the loader");
+ child.close();
+ child = null;
+ assertTrue(awaitCollected(ref), "loader must be collectable after the full
cleanup sequence");
+ }
+
+ private static boolean awaitCollected(WeakReference<?> ref) throws
InterruptedException {
+ for (int i = 0; i < 20; i++) {
+ if (ref.get() == null) {
+ return true;
+ }
+ System.gc();
+ Thread.sleep(50);
+ }
+ return ref.get() == null;
+ }
Review Comment:
The GC-based assertion window here is only ~1s and relies on `System.gc()`,
which is not guaranteed to collect promptly and can lead to flaky tests under
CI load. To reduce flakiness, consider increasing the timeout/backoff
substantially and/or switching to a more deterministic approach (e.g., longer
polling with a bounded max duration, or using a `ReferenceQueue`-backed pattern
to observe enqueueing).
##########
catalogs/catalog-common/src/test/java/org/apache/gravitino/utils/TestClassLoaderResourceCleanerUtils.java:
##########
@@ -68,4 +78,365 @@ 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 {
+ URL classesUrl =
LeakyValue.class.getProtectionDomain().getCodeSource().getLocation();
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+
+ WeakReference<ClassLoader> ref;
+ // Blocks the worker after it sets the ThreadLocal so the thread stays
alive; a thread that
+ // exits would null its own threadLocals in Thread.exit() and hide the
leak.
+ CountDownLatch release = new CountDownLatch(1);
+ Thread worker = null;
+ try {
+ URLClassLoader child = new URLClassLoader(new URL[] {classesUrl},
platform);
+ Object childOwnedValue =
+ Class.forName(LeakyValue.class.getName(), true, child)
+ .getDeclaredConstructor()
+ .newInstance();
+ // Guard the test's premise: the value must be loaded by `child`,
otherwise the sweep's
+ // value-ClassLoader check never matches and the reclamation below would
prove nothing.
+ assertSame(child, childOwnedValue.getClass().getClassLoader());
+ ref = new WeakReference<>(child);
+
+ CountDownLatch valueSet = new CountDownLatch(1);
+ Object[] valueHolder = {childOwnedValue};
+ worker =
+ new Thread(
+ () -> {
+ ThreadLocal<Object> tl = new ThreadLocal<>();
+ tl.set(valueHolder[0]);
+ valueSet.countDown();
+ try {
+ release.await();
+ } catch (InterruptedException ignored) {
+ // test teardown
+ }
+ },
+ "clp-reclaim-probe");
+ worker.setDaemon(true);
+ worker.start();
+ assertTrue(valueSet.await(10, TimeUnit.SECONDS), "worker should set its
ThreadLocal");
+
+ ClassLoaderResourceCleanerUtils.clearThreadLocalMap(worker, child);
+
+ // Drop every strong reference the test holds; the only thing that could
still pin `child` is
+ // the worker's ThreadLocal, which the sweep just cleared.
+ child.close();
+ child = null;
+ childOwnedValue = null;
+ valueHolder[0] = null;
+
+ // Assert before releasing the worker: once it returns, Thread.exit()
nulls its threadLocals
+ // and would let `child` be collected even if the sweep did nothing.
+ assertTrue(
+ awaitCollected(ref),
+ "dropped ClassLoader must be collectable once its ThreadLocal is
swept");
+ } finally {
+ release.countDown();
+ if (worker != null) {
+ worker.join(1000);
+ }
+ }
+ }
+
+ /**
+ * The property the issue is actually about: stopping the cleanup thread has
to make the dropped
+ * ClassLoader collectable, not merely make the thread go away. The control
case matters, because
+ * a bare WeakReference plus System.gc() assertion is unreliable enough that
a green result
+ * without one proves little.
+ */
+ @Test
+ void
testShutdownMySQLAbandonedConnectionCleanupThreadLetsDroppedClassLoaderBeCollected()
+ throws Exception {
+ URL mysqlJar = mysqlConnectorJarUrl();
+ Assumptions.assumeTrue(mysqlJar != null, "mysql-connector jar is not on
the test classpath");
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+
+ // Control: the same shape of child loader, never asked for the driver. If
this one were not
+ // collectable either, the assertions below would be measuring the probe,
not the leak.
+ URLClassLoader control = new URLClassLoader(new URL[] {mysqlJar},
platform);
+ WeakReference<ClassLoader> controlRef = new WeakReference<>(control);
+ control.close();
+ control = null;
+ assertTrue(
+ awaitCollected(controlRef),
+ "a child loader that never loaded the driver must be collectable");
+
+ URLClassLoader child = new URLClassLoader(new URL[] {mysqlJar}, platform);
+ Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread", true,
child);
+ assertNotNull(findCleanupThreadBoundTo(child), "the cleanup thread should
be running");
+ WeakReference<ClassLoader> ref = new WeakReference<>(child);
+ child.close();
+ child = null;
+
+ // The leak: the running cleanup thread pins the loader through its
context ClassLoader.
+ assertFalse(
+ awaitCollected(ref), "a live cleanup thread must keep the dropped
loader reachable");
+
+ // Still reachable, so the test can get the loader back to run the fix on
it.
+ ClassLoader leaked = ref.get();
+ assertNotNull(leaked, "loader should still be reachable while its cleanup
thread runs");
+
ClassLoaderResourceCleanerUtils.shutdownMySQLAbandonedConnectionCleanupThread(leaked);
+ assertTrue(
+ waitForCleanupThreadGone(leaked), "cleanup thread should be gone after
the shutdown");
+ leaked = null;
+
+ assertTrue(
+ awaitCollected(ref), "loader must be collectable once its cleanup
thread is stopped");
+ }
+
+ /**
+ * Drives the whole cleanup sequence rather than one step. Every other case
here calls a single
+ * step directly, so nothing covers the sequence, which is where step
ordering lives. This asserts
+ * the sequence's outcome; it cannot by itself distinguish one order from
another, since a later
+ * step can cover for an earlier one.
+ *
+ * <p>Goes through {@code runCleanupSteps} rather than {@code
closeClassLoaderResource}: the
+ * latter returns immediately when {@code GRAVITINO_TEST} is set, which the
build sets for every
+ * test task.
+ */
+ @Test
+ void testRunCleanupStepsStopsTheDriverThreadAndFreesTheClassLoader() throws
Exception {
+ URL mysqlJar = mysqlConnectorJarUrl();
+ Assumptions.assumeTrue(mysqlJar != null, "mysql-connector jar is not on
the test classpath");
+ ClassLoader platform = ClassLoader.getSystemClassLoader().getParent();
+
+ URLClassLoader child = new URLClassLoader(new URL[] {mysqlJar}, platform);
+ Class.forName("com.mysql.cj.jdbc.AbandonedConnectionCleanupThread", true,
child);
+ assertNotNull(findCleanupThreadBoundTo(child), "the cleanup thread should
be running");
+ WeakReference<ClassLoader> ref = new WeakReference<>(child);
+
+ ClassLoaderResourceCleanerUtils.runCleanupSteps(child);
+
+ assertTrue(
+ waitForCleanupThreadGone(child),
+ "the sequence should leave no cleanup thread on the loader");
+ child.close();
+ child = null;
+ assertTrue(awaitCollected(ref), "loader must be collectable after the full
cleanup sequence");
+ }
+
+ private static boolean awaitCollected(WeakReference<?> ref) throws
InterruptedException {
+ for (int i = 0; i < 20; i++) {
+ if (ref.get() == null) {
+ return true;
+ }
+ System.gc();
+ Thread.sleep(50);
+ }
+ return ref.get() == null;
+ }
+
+ private static URL mysqlConnectorJarUrl() {
+ // The driver class is on the test classpath (declared as
testImplementation). Resolve its
+ // code-source URL so a child URLClassLoader can load an isolated copy of
the driver.
+ try {
+ Class<?> driver = Class.forName("com.mysql.cj.jdbc.Driver");
+ return driver.getProtectionDomain().getCodeSource().getLocation();
+ } catch (Throwable t) {
+ return null;
+ }
+ }
Review Comment:
`Class.forName("com.mysql.cj.jdbc.Driver")` initializes the driver in the
test JVM, which can register with the global `DriverManager` and introduce
cross-test global side effects. It’s safer to avoid initialization here (e.g.,
`Class.forName(name, false, ...)`), or resolve the JAR URL via a class that
won’t trigger driver registration/initialization.
--
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]
