diff --git a/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/main/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClient.java b/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/main/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClient.java index 86e6dda16a43..81ba6c206712 100644 --- a/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/main/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClient.java +++ b/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/main/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClient.java @@ -83,11 +83,8 @@ public void initClient(final T config) { @Override public void close() { - if (!ObjectUtils.isEmpty(closeThread.get())) { - Runtime.getRuntime().removeShutdownHook(closeThread.get()); - } - if (isStarted.get()) { - isStarted.set(false); + this.tryRemoveShutdownHook(); + if (isStarted.compareAndSet(true, false)) { try { this.close0(); } catch (Exception e) { @@ -96,6 +93,35 @@ public void close() { } } + /** + * Remove the registered shutdown hook. When the JVM is already shutting down, + * the hook thread is running this {@code close()} itself, so removal throws + * {@link IllegalStateException} and is neither possible nor needed; cleanup + * must still proceed to {@code close0()}. + */ + protected void tryRemoveShutdownHook() { + final Thread hook = closeThread.get(); + if (ObjectUtils.isEmpty(hook)) { + return; + } + try { + removeHook(hook); + } catch (IllegalStateException e) { + LOG.debug("{} skips shutdown-hook removal: {}", this.getClass().getSimpleName(), e.getMessage()); + } + } + + /** + * Single point that talks to {@link Runtime}; protected so tests can simulate + * the {@code IllegalStateException("Shutdown in progress")} thrown when the JVM + * is already shutting down and the hook thread is running {@code close()} itself. + * + * @param hook the shutdown hook registered at init + */ + protected void removeHook(final Thread hook) { + Runtime.getRuntime().removeShutdownHook(hook); + } + @Override public void consume(final List logs) throws Exception { if (CollectionUtils.isEmpty(logs) || !isStarted.get()) { diff --git a/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/test/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClientTest.java b/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/test/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClientTest.java index 67089e3844b6..1d4143d47efd 100644 --- a/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/test/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClientTest.java +++ b/shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common/src/test/java/org/apache/shenyu/plugin/logging/common/client/AbstractLogConsumeClientTest.java @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -57,6 +58,36 @@ public void testConsumeRunsWhenInitializationSucceeds() throws Exception { assertEquals(1, client.getCloseCount()); } + @Test + public void testClose0StillRunsWhenHookRemovalFails() { + ThrowingHookLogConsumeClient client = new ThrowingHookLogConsumeClient(); + + try { + client.initClient(new GenericGlobalConfig()); + client.close(); + } finally { + Runtime.getRuntime().removeShutdownHook(client.getRegisteredHook()); + } + + assertEquals(1, client.getCloseCount()); + } + + @Test + public void testCloseIsIdempotent() { + TestLogConsumeClient client = new TestLogConsumeClient(true); + + try { + client.initClient(new GenericGlobalConfig()); + client.close(); + client.close(); + } finally { + // hook already removed by the first close(); removal is a no-op now + client.close(); + } + + assertEquals(1, client.getCloseCount()); + } + @Test public void testFailedReinitializationLeavesClientStopped() throws Exception { TestLogConsumeClient client = new TestLogConsumeClient(true); @@ -109,4 +140,44 @@ private int getCloseCount() { return closeCount; } } + + /** + * Simulates the JVM-shutdown scenario where {@code removeShutdownHook} throws + * {@link IllegalStateException}: cleanup must still reach {@code close0()}. + */ + private static final class ThrowingHookLogConsumeClient extends AbstractLogConsumeClient { + + private final AtomicReference registeredHook = new AtomicReference<>(); + + private int closeCount; + + @Override + public boolean initClient0(final GenericGlobalConfig config) { + registeredHook.set(new Thread(this::close)); + return true; + } + + @Override + public void consume0(final List logs) { + // not used in this test + } + + @Override + public void close0() { + closeCount++; + } + + @Override + protected void removeHook(final Thread hook) { + throw new IllegalStateException("Shutdown in progress"); + } + + private Thread getRegisteredHook() { + return registeredHook.get(); + } + + private int getCloseCount() { + return closeCount; + } + } }