Skip to content

Conversation

traggatt
Copy link
Contributor

@traggatt traggatt commented Aug 3, 2017

Fix for issue described in forum post: Acceptor dropping logout messages.

The issue exists in 1.6.x so would be good to merge there too.

Issue

When a FIX acceptor is shutdown, a call is made to SessionConnector.logoutAllSessions(). This calls Session.logout() on all sessions in turn, regardless of prior status. The logout() method is asynchronous and merely sets a private flag in Session. The actual logout message is sent later when the flag is picked up by a background thread (see Session.next()).

After the SessionConnector.logout() call completes, the SessionConnector should wait for all sessions to be logged out. However, due to incorrect use of SessionConnector.isLoggedOn(), waitForLogout() is never called.

isLoggedOn() performs an AND-style operation on the results of all session.isLoggedOn() methods. This means that if one or more sessions are not logged on, we do not wait for those that are.

Fix

The simple fix is to add a new method anyLoggedOn() to check if any of the FIX sessions are logged on. If so, then waitForLogout() is called and the required logout messages are sent before termination of the connection.

@traggatt
Copy link
Contributor Author

Thanks for the review. Will this be merged soon?

Incidentally, I'd be happy to raise another PR to merge this to the 1.6.x branch if you'd be open to the idea of a 1.6.5 version... :-D

@chrjohn chrjohn added this to the QFJ 1.7.0 milestone Aug 15, 2017
@chrjohn
Copy link
Member

chrjohn commented Aug 15, 2017

Please feel free to open a PR for 1.6.x. However, I cannot say when 1.6.5 will be released, if at all. At the moment the 1.7.0 release has priority.

@chrjohn chrjohn merged commit f82779d into quickfix-j:master Aug 15, 2017
@traggatt traggatt deleted the session-logout-fix branch August 15, 2017 08:22
@traggatt
Copy link
Contributor Author

Thanks for the merge. I've raised #131 which ports the fix to the 1.6.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants