paritytrading / philadelphia

Fast FIX protocol library for the JVM
Apache License 2.0
326 stars 96 forks source link

handleLogon() not triggered in case of unexpected msgSeqNum #315

Closed dmech closed 2 years ago

dmech commented 2 years ago

According to the FIX-specification, chapter "Synchronization after successful logon" --> Link the acknowledgment for the logon is transferred, before the resend request for possible missing message is sent. Philadelphia handles this well, except the logon message is not triggered in the case when msgSeqNum != rxMsgSeqNum

The sequence number comparison in the MessageHandler of FIXConnection could/should be extended with for example:

if (msgSeqNum != rxMsgSeqNum) {
    if (msgType.byteAt(0) == Logon) {
        handleLogon(message);
    }
    handleMsgSeqNum(message, msgType, msgSeqNum);
    return;
}
jvirtanen commented 2 years ago

Good catch, @dmech! 🔍

As I read the section, it looks though that this would only apply if msgSeqNum > rxMsgSeqNum, right? Nevertheless, we should fix the handling for that case.

I suggest adding the handling inside MessageHandler#handleMsgSeqNum() in order to keep it off the fast path of MessageHandler#message(), which we try to keep as simple as possible.

dmech commented 2 years ago

I rechecked the section and you are right, in this section the case msgSeqNum < rxMsgSeqNum is not mentioned, but in one section above it is:

If MsgSeqNum(34) is present in the incoming Logon(35=A) message and is less than NextNumIn, then a Logout(35=5) message should be sent assuming that an error exists in the state of either the initiator or acceptor FIX session processor, followed by a termination of the transport layer connection.

At the moment in this case (msgSeqNum < rxMsgSeqNum) a handleTooLowMsgSeqNum() is called. Looking at the quote above, for a Logon message, a handleLogout() should be called in MessageHandler#handleTooLowMsgSeqNum() not only for the Logoff case, but also for the Logon case.

Handling in MessageHandler#handleMsgSeqNum() is a good idea. For the implementation I found different approaches to identify a messagetype from a FIXValue:

which is the preferred way?

jvirtanen commented 2 years ago

The preferred way outside MessageHandler#message() is to use FIXValue#contentEquals(), which is more descriptive.

dmech commented 2 years ago

Thanks! May I create a proposal via pull request?

jvirtanen commented 2 years ago

Yes, please! 👍🏼

jvirtanen commented 2 years ago

Closed by #320.