You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@activemq.apache.org by cl...@apache.org on 2020/09/24 14:27:08 UTC
[activemq-artemis] 01/03: ARTEMIS-2912 Server start exception
before activation can cause a zombie broker
This is an automated email from the ASF dual-hosted git repository.
clebertsuconic pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/activemq-artemis.git
commit 207c2265bf58f98469c3a49ad7c7df7f47cefa78
Author: franz1981 <ni...@gmail.com>
AuthorDate: Wed Sep 23 23:37:11 2020 +0200
ARTEMIS-2912 Server start exception before activation can cause a zombie broker
---
.../apache/activemq/artemis/cli/commands/Run.java | 6 +--
.../artemis/core/server/ActiveMQServer.java | 11 ++++-
.../core/server/impl/ActiveMQServerImpl.java | 50 +++++++++++++++------
.../core/server/impl/ActiveMQServerImplTest.java | 51 ++++++++++++++++++++++
.../core/server/impl/EmbeddedServerTest.java | 8 ++--
.../cluster/failover/ReplicatedFailoverTest.java | 2 +-
6 files changed, 105 insertions(+), 23 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 a5b1487..366f0c0 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
@@ -88,7 +88,7 @@ public class Run extends LockAbstract {
AtomicBoolean serverActivationFailed = new AtomicBoolean(false);
server.getServer().registerActivationFailureListener(exception -> serverActivationFailed.set(true));
server.start();
- server.getServer().addExternalComponent(managementContext);
+ server.getServer().addExternalComponent(managementContext, false);
if (broker.web != null) {
broker.components.add(broker.web);
@@ -98,8 +98,8 @@ public class Run extends LockAbstract {
Class clazz = this.getClass().getClassLoader().loadClass(componentDTO.componentClassName);
ExternalComponent component = (ExternalComponent) clazz.newInstance();
component.configure(componentDTO, getBrokerInstance(), getBrokerHome());
- server.getServer().addExternalComponent(component);
- component.start();
+ server.getServer().addExternalComponent(component, true);
+ assert component.isStarted();
}
if (serverActivationFailed.get()) {
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 c0ee6c5..77ff502 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
@@ -833,7 +833,16 @@ public interface ActiveMQServer extends ServiceComponent {
void setMBeanServer(MBeanServer mBeanServer);
- void addExternalComponent(ActiveMQComponent externalComponent);
+ /**
+ * Adding external components is allowed only if the state
+ * isn't {@link SERVER_STATE#STOPPED} or {@link SERVER_STATE#STOPPING}.<br>
+ * It atomically starts the {@code externalComponent} while being added if {@code start == true}.<br>
+ * This atomicity is necessary to prevent {@link #stop()} to stop the component right after adding it, but before
+ * starting it.
+ *
+ * @throw IllegalStateException if the state is {@link SERVER_STATE#STOPPED} or {@link SERVER_STATE#STOPPING}
+ */
+ void addExternalComponent(ActiveMQComponent externalComponent, boolean start) throws Exception;
List<ActiveMQComponent> getExternalComponents();
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 c614571..46ba027 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
@@ -837,9 +837,23 @@ public class ActiveMQServerImpl implements ActiveMQServer {
this.mbeanServer = mbeanServer;
}
+ private void validateAddExternalComponent(ActiveMQComponent externalComponent) {
+ final SERVER_STATE state = this.state;
+ if (state == SERVER_STATE.STOPPED || state == SERVER_STATE.STOPPING) {
+ throw new IllegalStateException("cannot add " + externalComponent.getClass().getSimpleName() +
+ " if state is " + state);
+ }
+ }
+
@Override
- public void addExternalComponent(ActiveMQComponent externalComponent) {
- externalComponents.add(externalComponent);
+ public void addExternalComponent(ActiveMQComponent externalComponent, boolean start) throws Exception {
+ synchronized (externalComponents) {
+ validateAddExternalComponent(externalComponent);
+ externalComponents.add(externalComponent);
+ if (start) {
+ externalComponent.start();
+ }
+ }
}
@Override
@@ -1261,17 +1275,7 @@ public class ActiveMQServerImpl implements ActiveMQServer {
connectedClientIds.clear();
- for (ActiveMQComponent externalComponent : externalComponents) {
- try {
- if (externalComponent instanceof ServiceComponent) {
- ((ServiceComponent)externalComponent).stop(isShutdown || criticalIOError);
- } else {
- externalComponent.stop();
- }
- } catch (Exception e) {
- ActiveMQServerLogger.LOGGER.errorStoppingComponent(e, externalComponent.getClass().getName());
- }
- }
+ stopExternalComponents(isShutdown || criticalIOError);
try {
this.analyzer.clear();
@@ -4065,7 +4069,25 @@ public class ActiveMQServerImpl implements ActiveMQServer {
@Override
public List<ActiveMQComponent> getExternalComponents() {
- return externalComponents;
+ synchronized (externalComponents) {
+ return new ArrayList<>(externalComponents);
+ }
+ }
+
+ private void stopExternalComponents(boolean shutdown) {
+ synchronized (externalComponents) {
+ for (ActiveMQComponent externalComponent : externalComponents) {
+ try {
+ if (externalComponent instanceof ServiceComponent) {
+ ((ServiceComponent) externalComponent).stop(shutdown);
+ } 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/ActiveMQServerImplTest.java b/artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImplTest.java
index e443478..db3d07d 100644
--- a/artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImplTest.java
+++ b/artemis-server/src/test/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImplTest.java
@@ -26,9 +26,60 @@ import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
import org.junit.Assert;
import org.junit.Test;
+import static org.hamcrest.CoreMatchers.hasItem;
+import static org.hamcrest.collection.IsEmptyCollection.empty;
+
public class ActiveMQServerImplTest extends ActiveMQTestBase {
@Test
+ public void testAddingAndStartingExternalComponent() throws Exception {
+ ActiveMQServer server = createServer(false);
+ server.start();
+ EmbeddedServerTest.FakeExternalComponent component = new EmbeddedServerTest.FakeExternalComponent();
+ server.addExternalComponent(component, true);
+ Assert.assertTrue(component.isStarted());
+ Assert.assertThat(server.getExternalComponents(), hasItem(component));
+ }
+
+ @Test
+ public void testAddingWithoutStartingExternalComponent() throws Exception {
+ ActiveMQServer server = createServer(false);
+ server.start();
+ EmbeddedServerTest.FakeExternalComponent component = new EmbeddedServerTest.FakeExternalComponent();
+ server.addExternalComponent(component, false);
+ Assert.assertFalse(component.isStarted());
+ Assert.assertThat(server.getExternalComponents(), hasItem(component));
+ }
+
+ @Test
+ public void testCannotAddExternalComponentsIfNotStarting() throws Exception {
+ ActiveMQServer server = createServer(false);
+ EmbeddedServerTest.FakeExternalComponent component = new EmbeddedServerTest.FakeExternalComponent();
+ try {
+ server.addExternalComponent(component, false);
+ Assert.fail();
+ } catch (IllegalStateException ex) {
+ Assert.assertFalse(component.isStarted());
+ Assert.assertThat(server.getExternalComponents(), empty());
+ }
+ }
+
+ @Test
+ public void testCannotAddExternalComponentsIfStopped() throws Exception {
+ ActiveMQServer server = createServer(false);
+ server.start();
+ server.stop();
+ EmbeddedServerTest.FakeExternalComponent component = new EmbeddedServerTest.FakeExternalComponent();
+ try {
+ server.addExternalComponent(component, false);
+ Assert.fail();
+ } catch (IllegalStateException ex) {
+ Assert.assertFalse(component.isStarted());
+ Assert.assertThat(server.getExternalComponents(), empty());
+ }
+ }
+
+ @Test
public void testScheduledPoolGC() throws Exception {
ActiveMQServer server = createServer(false);
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 31b2626..cf07385 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
@@ -78,8 +78,8 @@ public class EmbeddedServerTest {
FakeExternalComponent normalComponent = new FakeExternalComponent();
FakeExternalServiceComponent serviceComponent = new FakeExternalServiceComponent();
- server.addExternalComponent(normalComponent);
- server.addExternalComponent(serviceComponent);
+ server.addExternalComponent(normalComponent, false);
+ server.addExternalComponent(serviceComponent, false);
server.stop(false);
assertTrue(normalComponent.stopCalled);
@@ -98,7 +98,7 @@ public class EmbeddedServerTest {
assertTrue(serviceComponent.exitCalled);
}
- private class FakeExternalComponent implements ActiveMQComponent {
+ public static class FakeExternalComponent implements ActiveMQComponent {
volatile boolean startCalled;
volatile boolean stopCalled;
@@ -124,7 +124,7 @@ public class EmbeddedServerTest {
}
}
- private class FakeExternalServiceComponent extends FakeExternalComponent implements ServiceComponent {
+ public static class FakeExternalServiceComponent extends FakeExternalComponent implements ServiceComponent {
volatile boolean exitCalled;
diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/ReplicatedFailoverTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/ReplicatedFailoverTest.java
index e246a8a..ab8b440 100644
--- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/ReplicatedFailoverTest.java
+++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/ReplicatedFailoverTest.java
@@ -176,7 +176,7 @@ public class ReplicatedFailoverTest extends FailoverTest {
backupServer.getServer().getNetworkHealthCheck().parseURIList("http://localhost:8787");
Assert.assertTrue(backupServer.getServer().getNetworkHealthCheck().isStarted());
- backupServer.getServer().addExternalComponent(webServerComponent);
+ backupServer.getServer().addExternalComponent(webServerComponent, false);
// this is called when backup servers go from live back to backup
backupServer.getServer().fail(true);
Assert.assertTrue(backupServer.getServer().getNetworkHealthCheck().isStarted());