Preventing the acceptor session from accepting Logon message when Logout was initiated locally.

[janusz-j]

Also see #359

When Logon sequence was initiated externally. i.e. counterparty initiated Logout sequence, acceptor should still accept subsequent Logon attempts from that counterparty.

[chrjohn]

Thanks for the PR. 👍

I haven't thought this through in all details but I think there might be a gap. I can think of at least one case where the session would not get re-enabled:
When the EndTime of the session is reached the Acceptor will send a Logout of its own, setting logoutInitiatedLocally = true and hence the session will not be re-enabled. Now when StartTime of the session is reached it will throw RejectLogon on each connection attempt and make it impossible to reconnect.

What do you think?

Edit: probably we need to track if setEnabled has been called manually?

[janusz-j]

Hi Chris.

I checked that and when the EndTime is reached, the acceptor calls Session.next(), which in turn (in line 1917) calls reset, which logs out the session, but without calling Session.logout(), so after the Logout triggered by the scheduler, the session is still enabled==true.
So that doesn't cause problems when the session turns "in schedule" again, acceptor accepts connections normally.

I manually tested that on my branch and it works as I described above.

Session.logout() is only called from client code (including JMX) and when (Threaded)SocketAcceptor/Initiator is stopped.

[chrjohn]

Hi Janusz,

sorry, my bad. I was working on a branch of PR #312 which is going to be merged in due course.
There the logout() method is called as you can see here: https://github.com/quickfix-j/quickfixj/pull/312/files#diff-f7029ca11a86ac97db29257d8e5582a8ed3e2e3242f07a1f13b919d430b6e207R884

[janusz-j]

OK, so this logout, when merged will break my fix.
Do you think PR #312 is OK?
Do I need to change mine, to get my problem solved?

[chrjohn]

Yes the other PR is okay. So we either need to find a way to solve the other problem or your problem differently.

[janusz-j]

OK. So when the other PR is merged, I'll try to fix it again.
Are you planning to merge it anytime soon?

[chrjohn]

Just merged #312

[janusz-j]

Thanks for letting me know Chris.
While I tried to make my changes work with the current version of master I also tried to understand the changes that were merged in #312.
I think the logout that you mentioned before: https://github.com/quickfix-j/quickfixj/pull/312/files#diff-f7029ca11a86ac97db29257d8e5582a8ed3e2e3242f07a1f13b919d430b6e207R884
is redundant there.
I removed it and all tests that were added in that PR still works.
No other changes done for that PR depends on that logout call in the reset method.

Also I read the conversation for that PR to understand more context, but it is still not clear to me why it was added.

Furthermore if the "enabled" attribute is supposed to do what the comment says:
/*

• Controls whether it is possible to log on to this Session (if Acceptor)
• or if Logon is sent out respectively (if Initiator).
  */
  resetting the session should not change its state.

I've manually checked some scenarios for my changes, but also for the changes in #312 and I can confirm, that the Logout is received before the disconnect when reset is called.
So the change introduced in #312 seems to still be working.

I changed my PR to remove the logout added in 312 and also improved my tests.

Please, let me know what you think.

[chrjohn]

Hi Janusz,
thanks for your changes. LGTM overall and good catch regarding the excess logout(). 👍 I'll give the unit test changes another thought but should be OK.
Cheers,
Chris.

[janusz-j]

Hi Chris.

Please let me know if you have anymore comments regarding this pull request.
Otherwise, when can I expect it to be merged and when the next release is planned?

Cheers,
Janusz.

[chrjohn]

Hi Janusz,
there still is one thing open, my suggested change from Feb 14 (see further above). But I guess the changes are OK with you. :)

Apart from that I still have some PRs in my pipeline that I want to push which I wanted to include in the release. Actually I wanted to create the release some weeks ago but too busy at the moment. :-/ Will try to get this done during the next weeks.

Cheers,
Chris.

[chrjohn]

NB: if one wanted to issue a Logout message without disabling the session, call Session.generateLogout().