Cleanup Sessions on stop

quickfixj-core/src/main/java/quickfix/Session.java

@@ -676,15 +676,23 @@ static void registerSession(Session session) {
         sessions.put(session.getSessionID(), session);
     }
 
-    static void unregisterSessions(List<SessionID> sessionIds) {
+    static void unregisterSessions(List<SessionID> sessionIds, boolean doClose) {
         for (final SessionID sessionId : sessionIds) {
-            final Session session = sessions.remove(sessionId);
-            if (session != null) {
-                try {
+            unregisterSession(sessionId, doClose);
+        }
+    }
+
+    static void unregisterSession(SessionID sessionId, boolean doClose) {
+        final Session session = sessions.get(sessionId);
+        if (session != null) {
+            try {
+                if (doClose) {
                     session.close();
-                } catch (final IOException e) {
-                    LOG.error("Failed to close session resources", e);
                 }
+            } catch (final IOException e) {
+                LOG.error("Failed to close session resources", e);
+            } finally {
+                sessions.remove(sessionId);
             }
         }
     }
@@ -2911,13 +2919,15 @@ public boolean isAllowedForSession(InetAddress remoteInetAddress) {
     }
 
     /**
-     * Closes session resources. This is for internal use and should typically
-     * not be called by an user application.
+     * Closes session resources and unregisters session. This is for internal
+     * use and should typically not be called by an user application.
      */
     @Override
     public void close() throws IOException {
         closeIfCloseable(getLog());
         closeIfCloseable(getStore());
+        // clean up session just in case close() was not called from Session.unregisterSession()
+        unregisterSession(this.sessionID, false);
     }
 
     private void closeIfCloseable(Object resource) throws IOException {

quickfixj-core/src/main/java/quickfix/SocketAcceptor.java

@@ -107,7 +107,8 @@ public void stop(boolean forceDisconnect) {
                 stopSessionTimer();
             } finally {
                 eventHandlingStrategy.stopHandlingMessages();
-                Session.unregisterSessions(getSessions());
+                Session.unregisterSessions(getSessions(), true);
+                clearConnectorSessions();
                 isStarted = Boolean.FALSE;
             }
         }

quickfixj-core/src/main/java/quickfix/SocketInitiator.java

@@ -96,7 +96,8 @@ public void stop(boolean forceDisconnect) {
                 stopInitiators();
             } finally {
                 eventHandlingStrategy.stopHandlingMessages();
-                Session.unregisterSessions(getSessions());
+                Session.unregisterSessions(getSessions(), true);
+                clearConnectorSessions();
                 isStarted = Boolean.FALSE;
             }
         }

quickfixj-core/src/main/java/quickfix/ThreadedSocketAcceptor.java

@@ -87,7 +87,8 @@ public void stop(boolean forceDisconnect) {
         }
         stopSessionTimer();
         eventHandlingStrategy.stopDispatcherThreads();
-        Session.unregisterSessions(getSessions());
+        Session.unregisterSessions(getSessions(), true);
+        clearConnectorSessions();
     }
 
     public void block() throws ConfigError, RuntimeError {

quickfixj-core/src/main/java/quickfix/ThreadedSocketInitiator.java

@@ -85,7 +85,8 @@ public void stop(boolean forceDisconnect) {
         logoutAllSessions(forceDisconnect);
         stopInitiators();
         eventHandlingStrategy.stopDispatcherThreads();
-        Session.unregisterSessions(getSessions());
+        Session.unregisterSessions(getSessions(), true);
+        clearConnectorSessions();
     }
 
     public void block() throws ConfigError, RuntimeError {

quickfixj-core/src/main/java/quickfix/mina/SessionConnector.java

@@ -119,6 +119,15 @@ protected void setSessions(Map<SessionID, Session> sessions) {
         propertyChangeSupport.firePropertyChange(SESSIONS_PROPERTY, null, sessions);
     }
 
+    /**
+     * Will remove all Sessions from the SessionConnector's Session map.
+     * Please make sure that these Sessions were unregistered before via
+     * Session.unregisterSessions().
+     */
+    protected void clearConnectorSessions() {
+        this.sessions.clear();
+    }
+
     /**
      * Get the list of session managed by this connector.
      *

quickfixj-core/src/main/java/quickfix/mina/initiator/AbstractSocketInitiator.java

@@ -77,9 +77,6 @@ protected AbstractSocketInitiator(SessionSettings settings, SessionFactory sessi
     protected void createSessionInitiators()
             throws ConfigError {
         try {
-            // QFJ698: clear() is needed on restart, otherwise the set gets filled up with
-            // more and more initiators which are not equal because the local port differs
-            initiators.clear();
             createSessions();
             SessionSettings settings = getSettings();
             for (final Session session : getSessionMap().values()) {
@@ -278,8 +275,9 @@ protected void startInitiators() {
     }
 
     protected void stopInitiators() {
-        for (final IoSessionInitiator initiator : initiators) {
-            initiator.stop();
+        for (Iterator<IoSessionInitiator> iterator = initiators.iterator(); iterator.hasNext();) {
+            iterator.next().stop();
+            iterator.remove();
         }
         super.stopSessionTimer();
     }

quickfixj-core/src/test/java/org/quickfixj/jmx/mbean/session/SessionAdminTest.java

@@ -20,11 +20,12 @@
 public class SessionAdminTest extends TestCase {
 
     public void testResetSequence() throws Exception {
-        Session session = SessionFactoryTestSupport.createSession();
-        MockSessionAdmin admin = new MockSessionAdmin(session, null, null);
-        admin.resetSequence(25);
-        assertEquals(1, admin.sentMessages.size());
-        assertEquals(25, admin.sentMessages.get(0).getInt(NewSeqNo.FIELD));
+        try (Session session = SessionFactoryTestSupport.createSession()) {
+            MockSessionAdmin admin = new MockSessionAdmin(session, null, null);
+            admin.resetSequence(25);
+            assertEquals(1, admin.sentMessages.size());
+            assertEquals(25, admin.sentMessages.get(0).getInt(NewSeqNo.FIELD));
+        }
     }
 
     private class MockSessionAdmin extends SessionAdmin {

quickfixj-core/src/test/java/quickfix/DefaultSessionFactoryTest.java

@@ -19,6 +19,7 @@
 
 package quickfix;
 
+import java.io.IOException;
 import org.junit.Before;
 import org.junit.Test;
 import quickfix.field.ApplVerID;
@@ -29,6 +30,7 @@
 
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
+import org.junit.After;
 import static org.junit.Assert.*;
 
 public class DefaultSessionFactoryTest {
@@ -45,6 +47,11 @@ public void setUp() throws Exception {
                 new SLF4JLogFactory(new SessionSettings()));
     }
 
+    @After
+    public void tearDown() {
+        Session.unregisterSession(sessionID, true);
+    }
+
     @Test
     public void testMinimalSettings() throws Exception {
         factory.create(sessionID, settings);
@@ -86,29 +93,31 @@ public void testFixtDataDictionaryConfiguration() throws Exception {
         settings.setString(sessionID, Session.SETTING_APP_DATA_DICTIONARY, "FIX42.xml");
         settings.setString(sessionID, Session.SETTING_APP_DATA_DICTIONARY + "." + FixVersions.BEGINSTRING_FIX40, "FIX40.xml");
 
-        Session session = factory.create(sessionID, settings);
-
-        DataDictionaryProvider provider = session.getDataDictionaryProvider();
-        assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
-                is(notNullValue()));
+        try (Session session = factory.create(sessionID, settings)) {
 
-        assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
-                is(notNullValue()));
-        assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX40)),
-                is(notNullValue()));
+            DataDictionaryProvider provider = session.getDataDictionaryProvider();
+            assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
+                    is(notNullValue()));
+
+            assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
+                    is(notNullValue()));
+            assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX40)),
+                    is(notNullValue()));
+        }
     }
 
     @Test
     public void testPreFixtDataDictionaryConfiguration() throws Exception {
         settings.setBool(sessionID, Session.SETTING_USE_DATA_DICTIONARY, true);
 
-        Session session = factory.create(sessionID, settings);
+        try (Session session = factory.create(sessionID, settings)) {
 
-        DataDictionaryProvider provider = session.getDataDictionaryProvider();
-        assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
-                is(notNullValue()));
-        assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
-                is(notNullValue()));
+            DataDictionaryProvider provider = session.getDataDictionaryProvider();
+            assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
+                    is(notNullValue()));
+            assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
+                    is(notNullValue()));
+        }
     }
 
     @Test
@@ -181,13 +190,15 @@ public void testIncorrectTimeValues() throws Exception {
     @Test
     public void testTestRequestDelayMultiplier() throws Exception {
         settings.setString(sessionID, Session.SETTING_TEST_REQUEST_DELAY_MULTIPLIER, "0.37");
-        Session session = factory.create(sessionID, settings);
-        assertEquals(0.37, session.getTestRequestDelayMultiplier(), 0);
+        try (Session session = factory.create(sessionID, settings)) {
+            assertEquals(0.37, session.getTestRequestDelayMultiplier(), 0);
+        }
     }
 
     private void createSessionAndAssertConfigError(String message, String pattern) {
+        Session session = null;
         try {
-            factory.create(sessionID, settings);
+            session = factory.create(sessionID, settings);
             fail(message);
         } catch (ConfigError e) {
             if (pattern != null) {
@@ -196,6 +207,14 @@ private void createSessionAndAssertConfigError(String message, String pattern) {
                 assertTrue("exception message not matched, expected: " + pattern + ", got: "
                         + e.getMessage(), m.matches());
             }
+        } finally {
+            if (session != null) {
+                try {
+                    session.close();
+                } catch (IOException ex) {
+                    // ignore
+                }
+            }
         }
     }
 
@@ -214,7 +233,8 @@ private void setUpDefaultSettings(SessionID sessionID) {
     @Test
     public void testReconnectIntervalInDefaultSession() throws Exception {
         settings.setString(sessionID, "ReconnectInterval", "2x5;3x15");
-        factory.create(sessionID, settings);
+        Session session = factory.create(sessionID, settings);
+        session.close();
     }
 
     @Test

quickfixj-core/src/test/java/quickfix/FileLogTest.java

@@ -202,14 +202,15 @@ public void testLogErrorWhenFilesystemRemoved() throws IOException {
         settings.setBool(sessionID, FileLogFactory.SETTING_INCLUDE_MILLIS_IN_TIMESTAMP, false);
         FileLogFactory factory = new FileLogFactory(settings);
 
-        Session session = new Session(new UnitTestApplication(), new MemoryStoreFactory(),
+        try (Session session = new Session(new UnitTestApplication(), new MemoryStoreFactory(),
                 sessionID, new DefaultDataDictionaryProvider(), null, factory,
-                new DefaultMessageFactory(), 0);
-        Session.registerSession(session);
-
-        FileLog log = (FileLog) session.getLog();
-        log.close();
-        log.logIncoming("test");
-        // no stack overflow exception thrown
+                new DefaultMessageFactory(), 0)) {
+            Session.registerSession(session);
+
+            FileLog log = (FileLog) session.getLog();
+            log.close();
+            log.logIncoming("test");
+            // no stack overflow exception thrown
+        }
     }
 }

quickfixj-core/src/test/java/quickfix/JdbcLogTest.java

@@ -26,14 +26,25 @@
 
 import javax.sql.DataSource;
 
-import junit.framework.TestCase;
-
-public class JdbcLogTest extends TestCase {
+import static junit.framework.TestCase.assertEquals;
+import static junit.framework.TestCase.assertNotNull;
+import static junit.framework.TestCase.assertTrue;
+import static junit.framework.TestCase.fail;
+import org.junit.After;
+import org.junit.Test;
+
+public class JdbcLogTest {
     private JdbcLog log;
     private JdbcLogFactory logFactory;
     private Connection connection;
     private SessionID sessionID;
 
+    @After
+    public void tearDown() {
+        Session.unregisterSession(sessionID, true);
+    }
+
+    @Test
     public void testLog() throws Exception {
         doLogTest(null);
     }
@@ -60,6 +71,7 @@ private void doLogTest(DataSource dataSource) throws ClassNotFoundException, SQL
         assertEquals(0, getRowCount(connection, "event_log"));
     }
 
+    @Test
     public void testLogWithHeartbeatFiltering() throws Exception {
         setUpJdbcLog(false, null);
 
@@ -83,6 +95,7 @@ public void testLogWithHeartbeatFiltering() throws Exception {
      * (such as we can't connect ot the DB, or the tables are missing) and doesn't try
      * to print failing exceptions recursively until the stack overflows
      */
+    @Test
     public void testHandlesRecursivelyFailingException() throws Exception {
         setUpJdbcLog(false, null);