From e09be4864aff04db372fb4c72c0d01c2fe91e798 Mon Sep 17 00:00:00 2001 From: Howard Gao Date: Thu, 23 Feb 2017 12:25:02 +0800 Subject: [PATCH] ARTEMIS-988 Regression: web tmp dir not cleaned up Due to recent changes, the web component is shutdown by the server, but the shutdown flag is lost so the web component's cleanup check method is not get called and the web's tmp dir is left there after user stopped the broker (control-c). The fix is add a suitable API to allow passing of the flag so the web component can make sure its tmp dir gets cleaned up properly before exiting the VM. --- .../activemq/artemis/cli/commands/Run.java | 4 +- .../artemis/integration/FileBroker.java | 10 ++- .../artemis/core/server/ServiceComponent.java | 3 +- .../artemis/core/server/ActiveMQServer.java | 2 +- .../core/server/impl/ActiveMQServerImpl.java | 36 ++++++--- .../core/server/impl/EmbeddedServerTest.java | 75 ++++++++++++++++++- .../artemis/component/WebServerComponent.java | 5 ++ 7 files changed, 118 insertions(+), 17 deletions(-) diff --git a/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Run.java b/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Run.java index 5efbef4c7b..1811365175 100644 --- a/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Run.java +++ b/artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Run.java @@ -134,7 +134,7 @@ public class Run extends LockAbstract { if (file.exists()) { try { try { - server.stop(true); + server.exit(); } catch (Exception e) { e.printStackTrace(); } @@ -155,7 +155,7 @@ public class Run extends LockAbstract { @Override public void run() { try { - server.stop(true); + server.exit(); } catch (Exception e) { e.printStackTrace(); } diff --git a/artemis-cli/src/main/java/org/apache/activemq/artemis/integration/FileBroker.java b/artemis-cli/src/main/java/org/apache/activemq/artemis/integration/FileBroker.java index 2776769425..03df935483 100644 --- a/artemis-cli/src/main/java/org/apache/activemq/artemis/integration/FileBroker.java +++ b/artemis-cli/src/main/java/org/apache/activemq/artemis/integration/FileBroker.java @@ -118,15 +118,19 @@ public class FileBroker implements Broker { } @Override - public void stop(boolean isShutdown) throws Exception { + public void exit() throws Exception { + stop(true); + } + + private void stop(boolean isShutdown) throws Exception { if (!started) { return; } ActiveMQComponent[] mqComponents = new ActiveMQComponent[components.size()]; components.values().toArray(mqComponents); for (int i = mqComponents.length - 1; i >= 0; i--) { - if (mqComponents[i] instanceof ServiceComponent) { - ((ServiceComponent)mqComponents[i]).stop(isShutdown); + if (mqComponents[i] instanceof ServiceComponent && isShutdown) { + ((ServiceComponent) mqComponents[i]).exit(); } else { mqComponents[i].stop(); } diff --git a/artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ServiceComponent.java b/artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ServiceComponent.java index 2fb0b6f309..6c347dd327 100644 --- a/artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ServiceComponent.java +++ b/artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ServiceComponent.java @@ -21,5 +21,6 @@ package org.apache.activemq.artemis.core.server; */ public interface ServiceComponent extends ActiveMQComponent { - void stop(boolean isShutdown) throws Exception; + //called by shutdown hooks before exit the VM + void exit() throws Exception; } diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java index bc020100fa..5798142867 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQServer.java @@ -64,7 +64,7 @@ import org.apache.activemq.artemis.utils.ExecutorFactory; *

* This is not part of our public API. */ -public interface ActiveMQServer extends ActiveMQComponent { +public interface ActiveMQServer extends ServiceComponent { /** * Sets the server identity. diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java index 0332137847..aa1ebf398e 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java @@ -130,6 +130,7 @@ import org.apache.activemq.artemis.core.server.QueueQueryResult; import org.apache.activemq.artemis.api.core.RoutingType; import org.apache.activemq.artemis.core.server.SecuritySettingPlugin; import org.apache.activemq.artemis.core.server.ServerSession; +import org.apache.activemq.artemis.core.server.ServiceComponent; import org.apache.activemq.artemis.core.server.ServiceRegistry; import org.apache.activemq.artemis.core.server.cluster.BackupManager; import org.apache.activemq.artemis.core.server.cluster.ClusterManager; @@ -335,7 +336,7 @@ public class ActiveMQServerImpl implements ActiveMQServer { @Override public void stop() throws Exception { - internalStop(); + internalStop(false); } @Override @@ -667,19 +668,24 @@ public class ActiveMQServerImpl implements ActiveMQServer { thread.start(); } + @Override + public void exit() throws Exception { + internalStop(true); + } + @Override public final void stop() throws Exception { + internalStop(false); + } + + private void internalStop(boolean isExit) throws Exception { try { - internalStop(); + stop(false, isExit); } finally { networkHealthCheck.stop(); } } - private void internalStop() throws Exception { - stop(false); - } - @Override public void addActivationParam(String key, Object val) { activationParams.put(key, val); @@ -810,7 +816,11 @@ public class ActiveMQServerImpl implements ActiveMQServer { @Override public final void stop(boolean failoverOnServerShutdown) throws Exception { - stop(failoverOnServerShutdown, false, false); + stop(failoverOnServerShutdown, false, false, false); + } + + public final void stop(boolean failoverOnServerShutdown, boolean isExit) throws Exception { + stop(failoverOnServerShutdown, false, false, isExit); } @Override @@ -830,12 +840,16 @@ public class ActiveMQServerImpl implements ActiveMQServer { } } + void stop(boolean failoverOnServerShutdown, final boolean criticalIOError, boolean restarting) { + this.stop(failoverOnServerShutdown, criticalIOError, restarting, false); + } + /** * Stops the server * * @param criticalIOError whether we have encountered an IO error with the journal etc */ - void stop(boolean failoverOnServerShutdown, final boolean criticalIOError, boolean restarting) { + void stop(boolean failoverOnServerShutdown, final boolean criticalIOError, boolean restarting, boolean isExit) { synchronized (this) { if (state == SERVER_STATE.STOPPED || state == SERVER_STATE.STOPPING) { @@ -1024,7 +1038,11 @@ public class ActiveMQServerImpl implements ActiveMQServer { for (ActiveMQComponent externalComponent : externalComponents) { try { - externalComponent.stop(); + if (isExit && externalComponent instanceof ServiceComponent) { + ((ServiceComponent)externalComponent).exit(); + } else { + externalComponent.stop(); + } } catch (Exception e) { ActiveMQServerLogger.LOGGER.errorStoppingComponent(e, externalComponent.getClass().getName()); } diff --git a/artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/EmbeddedServerTest.java b/artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/EmbeddedServerTest.java index 4b882cf3bb..e47440dcbe 100644 --- a/artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/EmbeddedServerTest.java +++ b/artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/EmbeddedServerTest.java @@ -24,13 +24,18 @@ import org.apache.activemq.artemis.api.core.TransportConfiguration; import org.apache.activemq.artemis.core.config.Configuration; import org.apache.activemq.artemis.core.config.impl.ConfigurationImpl; import org.apache.activemq.artemis.core.remoting.impl.invm.InVMAcceptorFactory; +import org.apache.activemq.artemis.core.server.ActiveMQComponent; import org.apache.activemq.artemis.core.server.ActiveMQServer; import org.apache.activemq.artemis.core.server.ActiveMQServers; +import org.apache.activemq.artemis.core.server.ServiceComponent; import org.junit.After; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + public class EmbeddedServerTest { private static final String SERVER_LOCK_NAME = "server.lock"; @@ -64,6 +69,74 @@ public class EmbeddedServerTest { public void testNoLockFileWithPersistenceFalse() { Path journalDir = Paths.get(SERVER_JOURNAL_DIR, SERVER_LOCK_NAME); boolean lockExists = Files.exists(journalDir); - Assert.assertFalse(lockExists); + assertFalse(lockExists); + } + + @Test + //make sure the correct stop/exit API is called. + public void testExternalComponentStop() throws Exception { + FakeExternalComponent normalComponent = new FakeExternalComponent(); + FakeExternalServiceComponent serviceComponent = new FakeExternalServiceComponent(); + + server.addExternalComponent(normalComponent); + server.addExternalComponent(serviceComponent); + + server.stop(); + assertTrue(normalComponent.stopCalled); + + assertTrue(serviceComponent.stopCalled); + assertFalse(serviceComponent.exitCalled); + + normalComponent.resetFlags(); + serviceComponent.resetFlags(); + + server.start(); + server.exit(); + assertTrue(normalComponent.stopCalled); + + assertFalse(serviceComponent.stopCalled); + assertTrue(serviceComponent.exitCalled); + } + + private class FakeExternalComponent implements ActiveMQComponent { + + volatile boolean startCalled; + volatile boolean stopCalled; + + @Override + public void start() throws Exception { + startCalled = true; + } + + @Override + public void stop() throws Exception { + stopCalled = true; + } + + @Override + public boolean isStarted() { + return startCalled; + } + + public void resetFlags() { + startCalled = false; + stopCalled = false; + } + } + + private class FakeExternalServiceComponent extends FakeExternalComponent implements ServiceComponent { + + volatile boolean exitCalled; + + @Override + public void exit() throws Exception { + exitCalled = true; + } + + @Override + public void resetFlags() { + super.resetFlags(); + exitCalled = false; + } } } diff --git a/artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java b/artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java index a6df2720c8..6dce708dc8 100644 --- a/artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java +++ b/artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java @@ -150,6 +150,7 @@ public class WebServerComponent implements ExternalComponent { //somehow when broker is stopped and restarted quickly //this tmpdir won't get deleted sometimes boolean fileDeleted = TimeUtils.waitOnBoolean(false, 5000, tmpdir::exists); + if (!fileDeleted) { //because the execution order of shutdown hooks are //not determined, so it's possible that the deleteOnExit @@ -190,6 +191,10 @@ public class WebServerComponent implements ExternalComponent { } @Override + public void exit() throws Exception { + stop(true); + } + public void stop(boolean isShutdown) throws Exception { if (isShutdown) { internalStop();