Skip to content

Commit 6b377f7

Browse files
chrjohnchrjohn
authored
Cleanup Sessions on stop (#175)
 - added logic to unregister Session to Session.close() method - changed some tests to use try-with-resources on Session - changed Connector implementations to clean up used Sessions on stop - changed AbstractSocketInitiator to clean up internal map of initiators on stop
1 parent 746a35b commit 6b377f7

21 files changed

+1712
-1569
lines changed

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

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -676,15 +676,23 @@ static void registerSession(Session session) {
676676
sessions.put(session.getSessionID(), session);
677677
}
678678

679-
static void unregisterSessions(List<SessionID> sessionIds) {
679+
static void unregisterSessions(List<SessionID> sessionIds, boolean doClose) {
680680
for (final SessionID sessionId : sessionIds) {
681-
final Session session = sessions.remove(sessionId);
682-
if (session != null) {
683-
try {
681+
unregisterSession(sessionId, doClose);
682+
}
683+
}
684+
685+
static void unregisterSession(SessionID sessionId, boolean doClose) {
686+
final Session session = sessions.get(sessionId);
687+
if (session != null) {
688+
try {
689+
if (doClose) {
684690
session.close();
685-
} catch (final IOException e) {
686-
LOG.error("Failed to close session resources", e);
687691
}
692+
} catch (final IOException e) {
693+
LOG.error("Failed to close session resources", e);
694+
} finally {
695+
sessions.remove(sessionId);
688696
}
689697
}
690698
}
@@ -2911,13 +2919,15 @@ public boolean isAllowedForSession(InetAddress remoteInetAddress) {
29112919
}
29122920

29132921
/**
2914-
* Closes session resources. This is for internal use and should typically
2915-
* not be called by an user application.
2922+
* Closes session resources and unregisters session. This is for internal
2923+
* use and should typically not be called by an user application.
29162924
*/
29172925
@Override
29182926
public void close() throws IOException {
29192927
closeIfCloseable(getLog());
29202928
closeIfCloseable(getStore());
2929+
// clean up session just in case close() was not called from Session.unregisterSession()
2930+
unregisterSession(this.sessionID, false);
29212931
}
29222932

29232933
private void closeIfCloseable(Object resource) throws IOException {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,8 @@ public void stop(boolean forceDisconnect) {
135135
stopSessionTimer();
136136
} finally {
137137
eventHandlingStrategy.stopHandlingMessages();
138-
Session.unregisterSessions(getSessions());
138+
Session.unregisterSessions(getSessions(), true);
139+
clearConnectorSessions();
139140
isStarted = Boolean.FALSE;
140141
}
141142
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,8 @@ public void stop(boolean forceDisconnect) {
124124
stopInitiators();
125125
} finally {
126126
eventHandlingStrategy.stopHandlingMessages();
127-
Session.unregisterSessions(getSessions());
127+
Session.unregisterSessions(getSessions(), true);
128+
clearConnectorSessions();
128129
isStarted = Boolean.FALSE;
129130
}
130131
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,8 @@ public void stop(boolean forceDisconnect) {
115115
}
116116
stopSessionTimer();
117117
eventHandlingStrategy.stopDispatcherThreads();
118-
Session.unregisterSessions(getSessions());
118+
Session.unregisterSessions(getSessions(), true);
119+
clearConnectorSessions();
119120
}
120121

121122
public void block() throws ConfigError, RuntimeError {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,8 @@ public void stop(boolean forceDisconnect) {
113113
logoutAllSessions(forceDisconnect);
114114
stopInitiators();
115115
eventHandlingStrategy.stopDispatcherThreads();
116-
Session.unregisterSessions(getSessions());
116+
Session.unregisterSessions(getSessions(), true);
117+
clearConnectorSessions();
117118
}
118119

119120
public void block() throws ConfigError, RuntimeError {

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,15 @@ protected void setSessions(Map<SessionID, Session> sessions) {
119119
propertyChangeSupport.firePropertyChange(SESSIONS_PROPERTY, null, sessions);
120120
}
121121

122+
/**
123+
* Will remove all Sessions from the SessionConnector's Session map.
124+
* Please make sure that these Sessions were unregistered before via
125+
* Session.unregisterSessions().
126+
*/
127+
protected void clearConnectorSessions() {
128+
this.sessions.clear();
129+
}
130+
122131
/**
123132
* Get the list of session managed by this connector.
124133
*

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,6 @@ protected AbstractSocketInitiator(SessionSettings settings, SessionFactory sessi
7777
protected void createSessionInitiators()
7878
throws ConfigError {
7979
try {
80-
// QFJ698: clear() is needed on restart, otherwise the set gets filled up with
81-
// more and more initiators which are not equal because the local port differs
82-
initiators.clear();
8380
createSessions();
8481
SessionSettings settings = getSettings();
8582
for (final Session session : getSessionMap().values()) {
@@ -278,8 +275,9 @@ protected void startInitiators() {
278275
}
279276

280277
protected void stopInitiators() {
281-
for (final IoSessionInitiator initiator : initiators) {
282-
initiator.stop();
278+
for (Iterator<IoSessionInitiator> iterator = initiators.iterator(); iterator.hasNext();) {
279+
iterator.next().stop();
280+
iterator.remove();
283281
}
284282
super.stopSessionTimer();
285283
}

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,12 @@
2020
public class SessionAdminTest extends TestCase {
2121

2222
public void testResetSequence() throws Exception {
23-
Session session = SessionFactoryTestSupport.createSession();
24-
MockSessionAdmin admin = new MockSessionAdmin(session, null, null);
25-
admin.resetSequence(25);
26-
assertEquals(1, admin.sentMessages.size());
27-
assertEquals(25, admin.sentMessages.get(0).getInt(NewSeqNo.FIELD));
23+
try (Session session = SessionFactoryTestSupport.createSession()) {
24+
MockSessionAdmin admin = new MockSessionAdmin(session, null, null);
25+
admin.resetSequence(25);
26+
assertEquals(1, admin.sentMessages.size());
27+
assertEquals(25, admin.sentMessages.get(0).getInt(NewSeqNo.FIELD));
28+
}
2829
}
2930

3031
private class MockSessionAdmin extends SessionAdmin {

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

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package quickfix;
2121

22+
import java.io.IOException;
2223
import org.junit.Before;
2324
import org.junit.Test;
2425
import quickfix.field.ApplVerID;
@@ -29,6 +30,7 @@
2930

3031
import static org.hamcrest.Matchers.is;
3132
import static org.hamcrest.Matchers.notNullValue;
33+
import org.junit.After;
3234
import static org.junit.Assert.*;
3335

3436
public class DefaultSessionFactoryTest {
@@ -45,6 +47,11 @@ public void setUp() throws Exception {
4547
new SLF4JLogFactory(new SessionSettings()));
4648
}
4749

50+
@After
51+
public void tearDown() {
52+
Session.unregisterSession(sessionID, true);
53+
}
54+
4855
@Test
4956
public void testMinimalSettings() throws Exception {
5057
factory.create(sessionID, settings);
@@ -86,29 +93,31 @@ public void testFixtDataDictionaryConfiguration() throws Exception {
8693
settings.setString(sessionID, Session.SETTING_APP_DATA_DICTIONARY, "FIX42.xml");
8794
settings.setString(sessionID, Session.SETTING_APP_DATA_DICTIONARY + "." + FixVersions.BEGINSTRING_FIX40, "FIX40.xml");
8895

89-
Session session = factory.create(sessionID, settings);
90-
91-
DataDictionaryProvider provider = session.getDataDictionaryProvider();
92-
assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
93-
is(notNullValue()));
96+
try (Session session = factory.create(sessionID, settings)) {
9497

95-
assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
96-
is(notNullValue()));
97-
assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX40)),
98-
is(notNullValue()));
98+
DataDictionaryProvider provider = session.getDataDictionaryProvider();
99+
assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
100+
is(notNullValue()));
101+
102+
assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
103+
is(notNullValue()));
104+
assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX40)),
105+
is(notNullValue()));
106+
}
99107
}
100108

101109
@Test
102110
public void testPreFixtDataDictionaryConfiguration() throws Exception {
103111
settings.setBool(sessionID, Session.SETTING_USE_DATA_DICTIONARY, true);
104112

105-
Session session = factory.create(sessionID, settings);
113+
try (Session session = factory.create(sessionID, settings)) {
106114

107-
DataDictionaryProvider provider = session.getDataDictionaryProvider();
108-
assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
109-
is(notNullValue()));
110-
assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
111-
is(notNullValue()));
115+
DataDictionaryProvider provider = session.getDataDictionaryProvider();
116+
assertThat(provider.getSessionDataDictionary(sessionID.getBeginString()),
117+
is(notNullValue()));
118+
assertThat(provider.getApplicationDataDictionary(new ApplVerID(ApplVerID.FIX42)),
119+
is(notNullValue()));
120+
}
112121
}
113122

114123
@Test
@@ -181,13 +190,15 @@ public void testIncorrectTimeValues() throws Exception {
181190
@Test
182191
public void testTestRequestDelayMultiplier() throws Exception {
183192
settings.setString(sessionID, Session.SETTING_TEST_REQUEST_DELAY_MULTIPLIER, "0.37");
184-
Session session = factory.create(sessionID, settings);
185-
assertEquals(0.37, session.getTestRequestDelayMultiplier(), 0);
193+
try (Session session = factory.create(sessionID, settings)) {
194+
assertEquals(0.37, session.getTestRequestDelayMultiplier(), 0);
195+
}
186196
}
187197

188198
private void createSessionAndAssertConfigError(String message, String pattern) {
199+
Session session = null;
189200
try {
190-
factory.create(sessionID, settings);
201+
session = factory.create(sessionID, settings);
191202
fail(message);
192203
} catch (ConfigError e) {
193204
if (pattern != null) {
@@ -196,6 +207,14 @@ private void createSessionAndAssertConfigError(String message, String pattern) {
196207
assertTrue("exception message not matched, expected: " + pattern + ", got: "
197208
+ e.getMessage(), m.matches());
198209
}
210+
} finally {
211+
if (session != null) {
212+
try {
213+
session.close();
214+
} catch (IOException ex) {
215+
// ignore
216+
}
217+
}
199218
}
200219
}
201220

@@ -214,7 +233,8 @@ private void setUpDefaultSettings(SessionID sessionID) {
214233
@Test
215234
public void testReconnectIntervalInDefaultSession() throws Exception {
216235
settings.setString(sessionID, "ReconnectInterval", "2x5;3x15");
217-
factory.create(sessionID, settings);
236+
Session session = factory.create(sessionID, settings);
237+
session.close();
218238
}
219239

220240
@Test

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

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -202,14 +202,15 @@ public void testLogErrorWhenFilesystemRemoved() throws IOException {
202202
settings.setBool(sessionID, FileLogFactory.SETTING_INCLUDE_MILLIS_IN_TIMESTAMP, false);
203203
FileLogFactory factory = new FileLogFactory(settings);
204204

205-
Session session = new Session(new UnitTestApplication(), new MemoryStoreFactory(),
205+
try (Session session = new Session(new UnitTestApplication(), new MemoryStoreFactory(),
206206
sessionID, new DefaultDataDictionaryProvider(), null, factory,
207-
new DefaultMessageFactory(), 0);
208-
Session.registerSession(session);
209-
210-
FileLog log = (FileLog) session.getLog();
211-
log.close();
212-
log.logIncoming("test");
213-
// no stack overflow exception thrown
207+
new DefaultMessageFactory(), 0)) {
208+
Session.registerSession(session);
209+
210+
FileLog log = (FileLog) session.getLog();
211+
log.close();
212+
log.logIncoming("test");
213+
// no stack overflow exception thrown
214+
}
214215
}
215216
}

0 commit comments

Comments
 (0)