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

Implement ZMTP 3.1 subscribe/cancel via commands #3814

Merged
merged 2 commits into from
Feb 7, 2020

Conversation

bluca
Copy link
Member

@bluca bluca commented Feb 4, 2020

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.

Copy link
Member

@sigiesec sigiesec left a 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.

@bluca bluca force-pushed the sub_cancel_decoder branch from e658699 to 370f2c0 Compare February 5, 2020 16:31
bluca added 2 commits February 5, 2020 17:03
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
@bluca bluca force-pushed the sub_cancel_decoder branch from 370f2c0 to 38b6888 Compare February 5, 2020 17:03
@bluca
Copy link
Member Author

bluca commented Feb 6, 2020

I wonder if we have tests that test for the ZMTP version negotiation and for communicating with older peers?

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

@sigiesec
Copy link
Member

sigiesec commented Feb 7, 2020

I'll add it to my to-do list as well ;)

@sigiesec sigiesec merged commit f17a794 into zeromq:master Feb 7, 2020
@bluca bluca deleted the sub_cancel_decoder branch February 7, 2020 08:57
@somdoron
Copy link
Member

somdoron commented Feb 8, 2020

You can start by porting the zmtp2 test of netmq:
https://github.com/zeromq/netmq/blob/master/src/NetMQ.Tests/ZMTPTests.cs#L27

@somdoron
Copy link
Member

somdoron commented Feb 8, 2020

reviewed it now, very nice implementation. No copy now...

@somdoron
Copy link
Member

somdoron commented Feb 8, 2020

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?

@somdoron
Copy link
Member

somdoron commented Feb 8, 2020

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.

@bluca
Copy link
Member Author

bluca commented Feb 8, 2020

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?

Here:

if (_manual || (options.type == ZMQ_XPUB && notify)) {

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.

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.

3 participants