Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wait for Logout response or timeout before disconnect on session reset #312

Merged
merged 2 commits into from
Feb 7, 2021
Merged

Wait for Logout response or timeout before disconnect on session reset #312

merged 2 commits into from
Feb 7, 2021

Conversation

simenflatby
Copy link
Contributor

@simenflatby simenflatby commented Sep 20, 2020

Fixes #244

This is my first attempt to contribute to the core QuickFIX/J codebase. Please read the described changes bellow and review the changes thoroughly.

Relevant changes

quickfix.Session#reset()

  • When there is a need to logout: Instead of calling quickfix.Session#generateLogout() followed by quickfix.Session#disconnect(String, boolean), set quickfix.Session#isResetStatePending flag and call quickfix.Session#logout(String) followed by quickfix.Session#generateLogout(String). quickfix.Session#logout(String) will set quickfix.Session#enabled to false and quickfix.Session#generateLogout(String) will send a Logout message. The flag quickfix.Session#isResetStatePending is checked when Logout response is received or when timing out waiting for a Logout response.
  • Only call quickfix.Session#resetState() if there is no need to logout.

quickfix.Session#nextLogout(Message)

  • Reset state also if quickfix.Session#isResetStatePending flag is set.

quickfix.Session#next()

  • Do not call quickfix.Session#reset() when outside of session time and quickfix.Session#isResetStatePending flag is set.
  • When outside of session time and reset is not needed and reset state is pending, call quickfix.Session#disconnect(String, boolean).
  • Check for Logout timeout before HeartBeat timeout.

quickfix.Session#disconnect(String, boolean)

  • Reset state also if quickfix.Session#isResetStatePending flag is set.

quickfix.Session#resetState()

  • Clear quickfix.Session#isResetStatePending flag.

quickfix.SessionTest#testLogonLogoutOnAcceptor()

  • Call quickfix.SessionTest#logoutFrom(Session, int). This is needed because we need to receive a Logout response in order to trigger the session reset.

quickfix.SessionTest#testStartOfInitiatorOutsideOfSessionTime()

  • Call quickfix.SessionTest#logoutFrom(Session, int). This is needed because we need to receive a Logout response in order to trigger the session reset. If we would like to simulate a Logout timeout instead since we are outside of session time, we could replace the call to quickfix.SessionTest#logoutFrom(Session, int) with systemTimeSource.increment(TimeUnit.HOURS.toMillis(state.getLogoutTimeout()) * 2); session.next();.

@chrjohn
Copy link
Member

chrjohn commented Sep 21, 2020

Thanks for the PR. I did not check the code yet but have one question: in SessionTest you are calling next() twice, does this mean one has to wait for two calls to next() at runtime also, i.e. max 2 seconds?

@simenflatby
Copy link
Contributor Author

simenflatby commented Sep 21, 2020

Thanks for the PR. I did not check the code yet but have one question: in SessionTest you are calling next() twice, does this mean one has to wait for two calls to next() at runtime also, i.e. max 2 seconds?

@chrjohn Looking at how quickfix.Session#next() is called now, I think you might be right. I will look at a solution tonight or one of the next days.

@simenflatby simenflatby changed the title ref #244 - Wait for Logout response or timeout before disconnect on session reset [WIP] ref #244 - Wait for Logout response or timeout before disconnect on session reset Sep 23, 2020
@simenflatby simenflatby changed the title [WIP] ref #244 - Wait for Logout response or timeout before disconnect on session reset Wait for Logout response or timeout before disconnect on session reset Sep 23, 2020
@simenflatby
Copy link
Contributor Author

@chrjohn I have now pushed an improved version. I squashed the new commit with the previous one so that you only have to review one. I have also edited the description of the pr. to reflect the new changes. Please review thoroughly when you can find the time.

Wait for Logout response or timeout before disconnect on session
reset.
@chrjohn chrjohn added the please review asking for a review label Oct 19, 2020
@chrjohn chrjohn linked an issue Oct 19, 2020 that may be closed by this pull request
@philipwhiuk
Copy link
Contributor

This looks okay to me - it doesn't fix #300 but we can probably build that on top...

@chrjohn
Copy link
Member

chrjohn commented Dec 17, 2020

@simenflatby thanks for this and 👍 for the extensive test coverage.
@philipwhiuk thanks for your review.

Currently not sure if I want to put this to the next patch release (to be released during the next weeks I think) since the state flags (i.e. the already existing ones) are hard to follow already and there were some problems around them during the last years. So will take an extra look later...

@simenflatby
Copy link
Contributor Author

@chrjohn My pleasure. I'm no longer with the company that I did the FIX integration that required this for, so I will not be able to test it in a real life project. If there is any questions, feel free to let me know and I will try to answer.

@philipwhiuk There has been some months since i looked at this code, so bear with me if I'm wrong: Would not calling quickfix.Session#generateLogout() solve #300? I would think that this will send a Logout message and that when the Logout message response is received in quickfix.Session#nextLogout(Message) it would trigger a disconnect?

@chrjohn chrjohn removed the please review asking for a review label Dec 29, 2020
@chrjohn chrjohn added this to the QFJ 2.2.1 milestone Dec 29, 2020
@chrjohn
Copy link
Member

chrjohn commented Dec 31, 2020

@simenflatby One question though: is there a specific reason why you used an AtomicBoolean for this? With the other AtomicBooleans that we use we do an atomic operation compareAndSet which you don't do with your isResetStatePending flag.

@simenflatby
Copy link
Contributor Author

simenflatby commented Jan 1, 2021

@chrjohn I can not remember, but from looking at it now, it looks like a sloppy copy-paste of the other state flags. It should probably be changed. I'm not in front of a dev computer for a while. Will you change it please? Otherwise I can do it after the weekend. Cheers!

@chrjohn
Copy link
Member

chrjohn commented Jan 2, 2021 via email

@chrjohn chrjohn self-requested a review January 6, 2021 23:21
@chrjohn
Copy link
Member

chrjohn commented Jan 17, 2021

@simenflatby , I think this state flag needs to be moved to the SessionState class and protected by its lock. See the other flags for reference, e.g. logonSent, logoutSent etc.
Please let me know if you still have the time to change this, otherwise I'll do it.
Thanks in advance 👍

@simenflatby
Copy link
Contributor Author

@chrjohn Unfortunately I have a lot on my schedule in January and February and I do not think I can find any time for OpenSource work. If it can wait until after that, I can definitely give it a shot. Just let me know!

@chrjohn chrjohn added the TODO label Jan 17, 2021
@chrjohn
Copy link
Member

chrjohn commented Jan 17, 2021

@simenflatby , no worries, I'll plan to do it then. Thanks for your work on this. 👍

@chrjohn chrjohn merged commit 17f0239 into quickfix-j:master Feb 7, 2021
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.

QFJ does not wait for Logout reply when EndTime is reached
3 participants