Copilot commented on code in PR #13249:
URL: https://github.com/apache/gravitino/pull/13249#discussion_r4026196978
##########
catalogs/catalog-common/src/test/java/org/apache/gravitino/utils/TestClassLoaderResourceCleanerUtils.java:
##########
@@ -325,4 +328,53 @@ void
testIsOwnedByClassLoaderReturnsFalseForBootstrapLoadedClass() {
ClassLoaderResourceCleanerUtils.isOwnedByClassLoader(
String.class, ClassLoader.getSystemClassLoader()));
}
+
+ @Test
+ void testClearShutdownHooksHoldsApplicationShutdownHooksMonitor() throws
Exception {
+ Class<?> shutdownHooksClass =
Class.forName("java.lang.ApplicationShutdownHooks");
+ ClassLoader targetLoader = new URLClassLoader(new URL[0], null);
+ Thread hookThread = new Thread(() -> {}, "cleaner-monitor-test-hook");
+ hookThread.setContextClassLoader(targetLoader);
+ Runtime.getRuntime().addShutdownHook(hookThread);
+ try {
+ CountDownLatch monitorHeld = new CountDownLatch(1);
+ Thread monitorHolder =
+ new Thread(
+ () -> {
+ synchronized (shutdownHooksClass) {
+ monitorHeld.countDown();
+ try {
+ Thread.sleep(600);
+ } catch (InterruptedException ignored) {
+ }
+ }
+ },
+ "cleaner-monitor-holder");
+ monitorHolder.start();
+ assertTrue(monitorHeld.await(5, TimeUnit.SECONDS));
+
+ long start = System.nanoTime();
+ ClassLoaderResourceCleanerUtils.clearShutdownHooks(targetLoader);
+ long elapsedMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() -
start);
+ monitorHolder.join();
+
+ // Before the fix, the hooks map was mutated without the
ApplicationShutdownHooks monitor,
+ // so this completed immediately even while another thread held the
monitor.
+ assertTrue(
+ elapsedMs >= 250,
Review Comment:
This regression test relies on a 600 ms sleep and a 250 ms wall-clock
threshold. Under CI scheduling or a pause, the holder can release the monitor
before the call starts, or the unfixed implementation can take more than 250 ms
and falsely pass. Use a release latch and a worker running
`clearShutdownHooks`, assert that the worker remains blocked while the monitor
is held, then release the monitor and assert completion/removal.
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/utils/ClassLoaderResourceCleanerUtils.java:
##########
@@ -301,20 +301,26 @@ 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}
synchronize on the
+ * {@code ApplicationShutdownHooks} class monitor, so the mutation must hold
that monitor too.
+ *
* @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;
- });
+ synchronized (shutdownHooks) {
+ hooks
+ .entrySet()
+ .removeIf(
+ entry -> {
+ Thread thread = entry.getKey();
+ return thread.getContextClassLoader() == targetClassLoader;
+ });
+ }
Review Comment:
The `hooks` reference is read before acquiring the
`ApplicationShutdownHooks` monitor. The JDK's shutdown path also uses this
monitor and sets the static field to `null`; if shutdown races this method,
this can leave a stale map that is no longer used or make `hooks.entrySet()`
throw a `NullPointerException`. Read the field, check for `null`, and remove
entries inside the synchronized block so this snapshot is atomic with the JDK's
transition.
--
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]
