LuciferYang commented on code in PR #13249:
URL: https://github.com/apache/gravitino/pull/13249#discussion_r4078521160
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -301,20 +301,36 @@ static boolean definedBy(@Nullable Object value,
ClassLoader classLoader) {
* <p>All shutdown hooks are run with the system class loader, so we need to
manually clear the
* shutdown hooks registered by the target class loader.
*
+ * <p>The map is JVM-global and {@code
ApplicationShutdownHooks.add/remove/runHooks} synchronize
+ * on the {@code ApplicationShutdownHooks} class monitor, so reading the
field, null-checking it,
+ * and mutating the map must all hold that monitor. {@code runHooks} nulls
the field under the
+ * monitor once shutdown starts, in which case there is nothing left for us
to clear.
+ *
* @param targetClassLoader the classloader where the shutdown hooks are
registered.
*/
- private static void clearShutdownHooks(ClassLoader targetClassLoader) throws
Exception {
+ @VisibleForTesting
+ static void clearShutdownHooks(ClassLoader targetClassLoader) throws
Exception {
Class<?> shutdownHooks =
Class.forName("java.lang.ApplicationShutdownHooks");
- IdentityHashMap<Thread, Thread> hooks =
- (IdentityHashMap<Thread, Thread>)
FieldUtils.readStaticField(shutdownHooks, "hooks", true);
-
- hooks
- .entrySet()
- .removeIf(
- entry -> {
- Thread thread = entry.getKey();
- return thread.getContextClassLoader() == targetClassLoader;
- });
+
+ // Read the field, null-check it, and mutate the map all under the same
monitor the JDK's
+ // add/remove/runHooks hold. During shutdown runHooks nulls the field
while holding this
+ // monitor, so reading it outside the lock could see a stale map or throw
an NPE.
+ synchronized (shutdownHooks) {
+ IdentityHashMap<Thread, Thread> hooks =
+ (IdentityHashMap<Thread, Thread>)
+ FieldUtils.readStaticField(shutdownHooks, "hooks", true);
+ if (hooks == null) {
Review Comment:
Not defensive, it is a real state the JDK produces.
`ApplicationShutdownHooks.hooks` is nulled under this same class monitor once
shutdown starts: `runHooks()` does `synchronized
(ApplicationShutdownHooks.class) { threads = hooks.keySet(); hooks = null; }`,
and the static initializer also leaves it null when the class is first loaded
after shutdown has already begun (which is why `add()`/`remove()` throw
`IllegalStateException("Shutdown in progress")` when it is null). So a
`clearShutdownHooks` call that races JVM shutdown genuinely observes null, and
when it does the JDK has taken ownership of the hooks and will run them itself,
so there is nothing left for us to clear and we return. Doing the read and
null-check inside the monitor (not outside) is what makes that observation
reliable rather than a stale read or an NPE.
--
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]
