ARTEMIS-222 fixing a deadlock that appeared on the testsuite (MultipleThreadsOpeningTest)

https://issues.apache.org/jira/browse/ARTEMIS-222

artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ServerLocatorImpl.java

@@ -561,13 +561,14 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery
       clusterTransportConfiguration = locator.clusterTransportConfiguration;
    }
 
-   private synchronized TransportConfiguration selectConnector() {
+   private TransportConfiguration selectConnector() {
       Pair<TransportConfiguration, TransportConfiguration>[] usedTopology;
 
       synchronized (topologyArrayGuard) {
          usedTopology = topologyArray;
       }
 
+      synchronized (this) {
          // if the topologyArray is null, we will use the initialConnectors
          if (usedTopology != null) {
             if (ActiveMQClientLogger.LOGGER.isTraceEnabled()) {
@@ -589,6 +590,7 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery
             return initialConnectors[pos];
          }
       }
+   }
 
    public void start(Executor executor) throws Exception {
       initialise();
@@ -637,17 +639,24 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery
    }
 
    private ClientSessionFactoryInternal connect(final boolean skipWarnings) throws ActiveMQException {
+      ClientSessionFactoryInternal returnFactory = null;
+
       synchronized (this) {
          // static list of initial connectors
          if (getNumInitialConnectors() > 0 && discoveryGroup == null) {
-            ClientSessionFactoryInternal sf = (ClientSessionFactoryInternal) staticConnector.connect(skipWarnings);
+            returnFactory = (ClientSessionFactoryInternal) staticConnector.connect(skipWarnings);
-            addFactory(sf);
-            return sf;
          }
       }
+
+      if (returnFactory != null) {
+         addFactory(returnFactory);
+         return returnFactory;
+      }
+      else {
          // wait for discovery group to get the list of initial connectors
          return (ClientSessionFactoryInternal) createSessionFactory();
       }
+   }
 
    @Override
    public ClientSessionFactoryInternal connectNoWarnings() throws ActiveMQException {
@@ -844,12 +853,12 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery
                factory.cleanup();
             throw ActiveMQClientMessageBundle.BUNDLE.connectionTimedOutOnReceiveTopology(discoveryGroup);
          }
+      }
 
       addFactory(factory);
 
       return factory;
    }
-   }
 
    public boolean isHA() {
       return ha;
@@ -1494,10 +1503,13 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery
    }
 
    public void factoryClosed(final ClientSessionFactory factory) {
+      boolean isEmpty;
       synchronized (factories) {
          factories.remove(factory);
+         isEmpty = factories.isEmpty();
+      }
 
-         if (!clusterConnection && factories.isEmpty()) {
+      if (!clusterConnection && isEmpty) {
          // Go back to using the broadcast or static list
          synchronized (topologyArrayGuard) {
             receivedTopology = false;
@@ -1506,7 +1518,6 @@ public final class ServerLocatorImpl implements ServerLocatorInternal, Discovery
          }
       }
    }
-   }
 
    public Topology getTopology() {
       return topology;

tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/cluster/MultipleThreadsOpeningTest.java

@@ -23,8 +23,8 @@ import java.util.concurrent.CountDownLatch;
 import org.apache.activemq.artemis.api.core.TransportConfiguration;
 import org.apache.activemq.artemis.api.jms.ActiveMQJMSClient;
 import org.apache.activemq.artemis.api.jms.JMSFactoryType;
-import org.apache.activemq.artemis.tests.util.JMSClusteredTestBase;
 import org.apache.activemq.artemis.core.remoting.impl.invm.InVMConnectorFactory;
+import org.apache.activemq.artemis.tests.util.JMSClusteredTestBase;
 import org.junit.Test;
 
 public class MultipleThreadsOpeningTest extends JMSClusteredTestBase {
@@ -33,7 +33,7 @@ public class MultipleThreadsOpeningTest extends JMSClusteredTestBase {
    public void testMultipleOpen() throws Exception {
       cf1 = ActiveMQJMSClient.createConnectionFactoryWithHA(JMSFactoryType.CF, new TransportConfiguration(InVMConnectorFactory.class.getName(), generateInVMParams(1)));
 
-      final int numberOfOpens = 2000;
+      final int numberOfOpens = 500;
       int numberOfThreads = 20;
       // I want all the threads aligned, just ready to start creating connections like in a car race
       final CountDownLatch flagAlignSemaphore = new CountDownLatch(numberOfThreads);
@@ -41,6 +41,10 @@ public class MultipleThreadsOpeningTest extends JMSClusteredTestBase {
 
       class ThreadOpen extends Thread {
 
+         ThreadOpen(int i) {
+            super("MultipleThreadsOpeningTest/ThreadOpen::" + i);
+         }
+
          int errors = 0;
 
          public void run() {
@@ -50,8 +54,8 @@ public class MultipleThreadsOpeningTest extends JMSClusteredTestBase {
                flagStartRace.await();
 
                for (int i = 0; i < numberOfOpens; i++) {
-                  if (i % 1000 == 0)
+                  if (i % 100 == 0)
-                     System.out.println("tests " + i);
+                     System.out.println("connections created on Thread " + Thread.currentThread() + " " + i);
                   Connection conn = cf1.createConnection();
                   Session sess = conn.createSession(true, Session.AUTO_ACKNOWLEDGE);
                   sess.close();
@@ -68,18 +72,27 @@ public class MultipleThreadsOpeningTest extends JMSClusteredTestBase {
       ThreadOpen[] threads = new ThreadOpen[numberOfThreads];
 
       for (int i = 0; i < numberOfThreads; i++) {
-         threads[i] = new ThreadOpen();
+         threads[i] = new ThreadOpen(i);
          threads[i].start();
       }
 
       flagAlignSemaphore.await();
       flagStartRace.countDown();
 
+      try {
          for (ThreadOpen t : threads) {
-         // 5 minutes seems long but this may take a bit of time in a slower box
+            t.join(60000);
-         t.join(300000);
             assertFalse(t.isAlive());
             assertEquals("There are Errors on the test thread", 0, t.errors);
          }
       }
+      finally {
+         for (ThreadOpen t : threads) {
+            if (t.isAlive()) {
+               t.interrupt();
+            }
+            t.join(1000);
+         }
+      }
+   }
 }