-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Implement ZMTP 3.1 subscribe/cancel via commands #3814
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this finally gets implemented!
I wonder if we have tests that test for the ZMTP version negotiation and for communicating with older peers?
Please see my inline remarks, which, for the most part, address the understandability of the comments.
e658699
to
370f2c0
Compare
Solution: if all peers of a socket are >= 3.1 use sub/cancel commands instead of the old 0/1 messages. For backward compatibility, move the handling of 0/1 or sub/cancel command strings to the encoders, so that the right thing can be done depending on the protocol version. Do not set the command flag until the encoder, so that we can handle the inproc case (which skips the encoder).
Solution: bump minor version number in the engine as all 3.1 features are now implemented
370f2c0
to
38b6888
Compare
Unfortunately we don't - it's been on my to-do list forever to add cross-compat tests with the various zmtp implementations, but never had the time to do it |
I'll add it to my to-do list as well ;) |
You can start by porting the zmtp2 test of netmq: |
reviewed it now, very nice implementation. No copy now... |
I think it might break the API, so when decoding, where do you convert a subscriber/join command to a msg with 0/1 at the beginning? If you are not, the existing implementations of XPUB might break. What do you think? |
I think we should move the handling of the cancel/subscribe commands from the zmtp_engine to v31_decoder. And those should remove the command_name from the data and leave only the data with the flags of subscribe/join. I think commands should be engine only thing and should not travel up to the sockets. |
Here: Line 163 in f17a794
I'd prefer to keep the internal representation using flags metadata too, rather than the 0/1 byte. That way in a distant future where zmtp < 3.1 is no longer supported, we just remove the conversion in xpub. It's also cleaner to use flags when reasoning about objects, rather than having to look at the data. |
Thanks @somdoron for the suggestion to move the handling of the flag/command to the engine. Backward compatibility is retained. The drawback is that if a single sub/unsub is sent on a sub connected to many pubs, the work will be redone everytime. But it's a one byte or 7/11 bytes fixed size buffer, so it shouldn't make much of a difference, as it's just adding to an existing 2 bytes buffer.