[#12123] fix(catalog): shut down MySQL AbandonedConnectionCleanupThread and broaden ThreadLocal cleanup to prevent Metaspace leak#12124
Open
LuciferYang wants to merge 3 commits into
Conversation
… and broaden ThreadLocal cleanup to prevent Metaspace leak on catalog reload closeClassLoaderResource now shuts down MySQL's AbandonedConnectionCleanupThread (via uncheckedShutdown, which also clears its tracked-connection map) and sweeps ThreadLocals on all application threads instead of only Gravitino-webserver-*. Both release references that otherwise pin a dropped catalog's isolated ClassLoader and leak Metaspace on reload. The MySQL shutdown is guarded by isOwnedByClassLoader, matching the other destructive steps in this class, so it never stops a cleanup thread owned by a parent/shared ClassLoader (e.g. a MySQL entity store on the app ClassLoader). Add TestClassLoaderResourceCleanerUtils cases covering the owned-loader shutdown, the parent-owned skip, the broadened ThreadLocal sweep (target-CL value cleared, other non-null-CL value kept), and the system-group skip; mysql-connector-j added as a test-only dependency.
yuqi1129
reviewed
Jul 21, 2026
| // 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())) { |
Contributor
There was a problem hiding this comment.
Have you tested and verified that it works or not? I'm not sure whether removing logic Gravitino-webserver and replacing it with the thread group can function well when the catalog is closed.
Contributor
Author
There was a problem hiding this 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
stopThreadsAndClearThreadLocalVariablesis still guarded byrunningWithClassLoader(contextClassLoader == target), unchanged. And an entry is only cleared whenvalue.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
ThreadLocalMapaccess needs--add-opens java.base/java.lang=ALL-UNNAMED, whichbin/gravitino.sh.templatealready sets, so it works at runtime.
./gradlew :catalogs:catalog-common:test --tests "*TestClassLoaderResourceCleanerUtils": 8/8 green.
Code Coverage Report
Files
|
…able after the broadened ThreadLocal sweep Add a WeakReference-based test that reproduces the leak on a long-lived non-webserver thread holding a ThreadLocal whose value is loaded by the target ClassLoader, then asserts the ClassLoader is collected once clearThreadLocalMap runs. The test fails under the previous webserver-only filter.
- assert the ThreadLocal value is loaded by the target ClassLoader, so a no-op sweep can no longer pass the test - bound valueSet.await with a timeout so a stuck worker fails instead of hanging - move the reclaim assertion inside try and release/join in finally, keeping assert-before-release so the worker cannot exit and clear its own threadLocals before the assertion - drop the no-op Runtime.runFinalization() and a tautological assertion
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Two additions to
ClassLoaderResourceCleanerUtils.closeClassLoaderResource:AbandonedConnectionCleanupThreadviauncheckedShutdown(), which also clears the tracked-connection map so the driver's ClassLoader can be collected. The call is guarded byisOwnedByClassLoader, matching the other cleanup steps in this class, so it never stops a cleanup thread owned by a parent or shared ClassLoader (for example a MySQL entity store loaded on the app ClassLoader, whose driver class delegates up to the app loader).Gravitino-webserver-*threads to all application threads, skipping only the JVMsystemthread group. Only entries whose value was loaded by the target ClassLoader are cleared, so ThreadLocals owned by other ClassLoaders are left alone.Why are the changes needed?
After a MySQL-backed catalog is dropped, its isolated ClassLoader stays reachable through the
mysql-cj-abandoned-connection-cleanupthread's context ClassLoader and through ThreadLocals on non-webserver threads (Caffeine ForkJoinPool, catalog-cleaner, Hadoop daemons).closeClassLoaderResourcereleases neither today, so the ClassLoader leaks on every reload and Metaspace grows untilOutOfMemoryError: Metaspace.Fix: #12123
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added
TestClassLoaderResourceCleanerUtilscases (mysql-connector-jadded as a test-only dependency):testShutdownMySQLAbandonedConnectionCleanupThreadStopsThread: loadingAbandonedConnectionCleanupThreadin a childURLClassLoaderstarts the cleanup thread bound to that loader; aftershutdownMySQLAbandonedConnectionCleanupThreadthe thread is gone. It loads the cleanup-thread class directly rather than the driver, so the test does not register a driver into the JVM-globalDriverManager.testShutdownMySQLAbandonedConnectionCleanupThreadSkipsParentOwnedClass: when the class resolves to a parent loader, a child-scoped cleanup leaves the parent's thread alive; an owner-scoped cleanup stops it.testClearThreadLocalMapClearsOnlyTargetClassLoaderValues: on a non-webserver thread, a ThreadLocal whose value is loaded by the target ClassLoader is cleared, while one loaded by a different (non-null) ClassLoader is kept.testClearThreadLocalMapSkipsSystemThreadGroup: a ThreadLocal on asystem-group thread is left untouched.Ran
./gradlew :catalogs:catalog-common:test --tests "org.apache.gravitino.utils.TestClassLoaderResourceCleanerUtils": 7/7 green.