Problem: can't process ZMTP 3.1 cancel/subscribe commands #3168
Conversation
|
So you intend to add some tests for this when the send side is also added? I must admit that I cannot really judge the additions. Should someone else have a look before merging? Otherwise, I will merge it. |
src/msg.cpp
Outdated
| data = | ||
| static_cast<unsigned char *> (this->data ()) + cancel_cmd_name_size; | ||
|
|
||
| return static_cast<void *> (data); |
There was a problem hiding this comment.
I think this cast is redundant.
src/msg.hpp
Outdated
| @@ -39,6 +39,9 @@ | |||
| #include "atomic_counter.hpp" | |||
| #include "metadata.hpp" | |||
|
|
|||
| // bits 2-5 | |||
| #define CMD_TYPE_MASK 28 | |||
There was a problem hiding this comment.
maybe define this as a hex literal instead to make the relationship to the bits more obvious?
In c++ style, a constant would be preferable to a preprocessor definition.
There was a problem hiding this comment.
the problem is storage - msg has to be 64 bytes, but adding a variable would add weight to that, right?
src/xpub.cpp
Outdated
|
|
||
| // ZMTP 3.1 hack: we need to support sub/cancel commands, but | ||
| // we can't give them back to userspace as it would be an API | ||
| // breackage since the payload of the message is completely |
Yeah it's a bit hard to do without the sending side - I can try to mock it |
|
I think the best were to have tests for the receiver side only as unittests. However, we do not have any comparable tests until now, so probably this would require quite significant changes to the affected classes to make them unit-testable. So if you intend to add support for the send side in the near future, and then add API-level tests, this should be fine. Then it is at the same level of tests as comparable existing features. |
|
I am confused, since the rfc document defined:
If the sub side send "SUB A" to pub side twice, we store the pipe of sub into same node of struct trie, and save into a struct of set inner the node. The behavior is different from the definition of rfc doc. I am not sure whether I'm wrong or not. |
|
That is enforced on the sub side - if you send "subscribe A" "subscribe A" and then "cancel A" "cancel A", the sub will wait for the second cancel before sending |
Solution: list them
Solution: fix path in Makefile.am
Solution: do it to silence static analysis warnings
|
Which I guess was done to reduce traffic - but it does breach the protocol. But at this point other implementations are probably doing the same thing, so we can't really change that. Perhaps the RFC should be changed instead to reflect the actual implementations... |
|
@sigiesec I've added a simple mock test |
Solution: use the --version-script map on those systems as well, as it is supported
Solution: add some msg helpers to parse commands, and check for subscribe or cancel commands and process them accordingly in the xpub and xsub classes.
|
Sorry I was AFK for the last few days. Thanks for the test. So is this ready to merge in spite of the discussion on the protocol? |
|
No worries! Yeah I'm not changing the way the PUB-SUB subscription behaviour works here - it was already like that. This just adds the new on-the-wire format. |
Solution: fix regression introduced by: zeromq#3168 Correctly fall back to user message if the first byte is neither 0 nor 1, and add a simple unit test
Solution: fix regression introduced by: zeromq#3168 Correctly fall back to user message if the first byte is neither 0 nor 1, and add a simple unit test Fixes zeromq#3656
There's a bit of a hack to manage to use the existing flag field in msg - it works since a msg can't be more than one command at any time.
This is just to process on receive - to send, it's a bit more complicated so I'll send separately