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

Fix next_message_reader() method #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Roman-Koshelev
Copy link

According to the comment, the function must return a valid message
(although not necessarily complete). This is logical, but not so now.
Also, now, in the absence of a valid message, we will return a zero-size
reader pointing to the last byte

According to the comment, the function must return a valid message
(although not necessarily complete). This is logical, but not so now.
Also, now, in the absence of a valid message, we will return a zero-size
reader pointing to the last byte
@jamesdbrock
Copy link
Owner

jamesdbrock commented Apr 17, 2021

Please explain further? How is next_message_reader supposed to behave after this PR?

Honestly, this whole business with automatically searching for the next 8=FIX string after an invalid message was probably a bad idea to begin with. Today I would not recommend that anyone use that feature. If a user encounters some invalid FIX, my advice would be to stop parsing, stop trading, close the connection, and get on the phone immediately with the person who sent the invalid FIX.

Users can write their own function to scan through invalid garbage bytes looking for some valid FIX, if they really want to. Which might be a good idea sometimes, like when the user is trying to parse and audit old trading logs. But choosing which kinds of garbage are allowable is very specific to the situation, and this library shouldn't guess about that.

@Roman-Koshelev
Copy link
Author

I agree with you, but I think that this will have to stay at least for the sake of backward compatibility.

  1. In the current implementation, we are simply looking for "8=FIX". This contradicts
    " if there might be a complete or partial valid message anywhere else in the remainder of the buffer, will return a new message_reader constructed at that location.".
    I think if we find the first occurrence at the possible beginning of the FIX message and it is not the beginning, then we must continue the search.
  2. If there is no partial or complete valid message in the rest of the buffer, we must return an empty buffer pointing to the last byte. In the current implementation, we return the reader to the last 10 bytes. What does it mean? what does begin () of this buffer point to? Unclear...

@jamesdbrock
Copy link
Owner

If there is no valid message in the rest of the buffer, then we don't know where the last byte is.

That's why the message_reader::end() says

std::logic_error if called on an invalid message. This exception is preventable by program logic. You should always check if a message is_valid() before reading.

https://jamesdbrock.github.io/hffix/classhffix_1_1message__reader.html#a766234db0fadea164a1e4f71f49af381

This is awkward, but it's pretty common practice in C++. It would be better if we could make this invalid state unrepresentable so that the compiler could verify that we never call end() on an invalid message, but I don't know how to do that in a language like C++ which has no algebraic sum types.

@Roman-Koshelev
Copy link
Author

message_begin and prefix_* does not throw an exception. I still find the change very useful

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