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

Fixed ZMQ_REQ_CORRELATE (see pull request #1730) #1882

Merged
merged 1 commit into from
Apr 2, 2016

Conversation

FredTreg
Copy link
Contributor

@FredTreg FredTreg commented Apr 2, 2016

Commit Comment
Problem: Since pull request #1730 was merged, protocol for REQ socket is checked at the session level and this check does not take into account the possibility of a request_id being part of the message. Thus the option ZMQ_REQ_CORRELATE would no longer work.
This is now fixed: the possiblity of a 4 bytes integer being present before the delimiter frame is taken into account (whether or not this breaks the REQ/REP RFC is another issue).

Work done

  • Added a possible state request_id in the protocol of the REQ socket
  • Removed the timers in test_req_relaxed as they were there only to make the tests fail fast
  • Removed the tests in test_req_correlate that were testing that invalid REQ messages were silently ignored. This is no longer the case: malformed messages now trigger the closing of the peer that sent them.

Comments

  • The protocol for an incoming message accepted by the REQ is now either 0|payload orrequest_id|0|payload. I did not bother checking whether the ZMQ_REQ_CORRELATE was on or not as I found too cumbersome to check this at the session level (would have to pass the option from the socket to the session). I do not think that this is too much of a problem: we may accept invalid message in some cases but then they would be silently ignored at the socket level.
  • As discussed on pull request problem: request socket state machine check is not running #1730, an alternative solution would have been to remove the checks done at the session level and keep the checking mechanism done at the socket level. But the libzmq project team advised against it.
  • As the checks of the REQ protocol are now done at the session level, the same should probably be done for the REP socket (indeed why would the REQ socket be the only socket checking that its protocol is valid at the session level when the REP has exactly the same protocol and does its checking as the socket level?)
  • There is still the question of the REQREP RFC (the possibility of a request_id as a first frame of the REQREP protocol is not described in the RFC).
  • As I mentionned in defect ZMQ_REQ_CORRELATE may not work when sent messages are not received immediately #1695, I came to believe that ZMQ_REQ_CORRELATE and ZMQ_REQ_RELAXED serve very little purpose but generate a lot of line of codes in libzmq. If at some point in a given workflow, someone needs to send two messages in a row from a REQ socket, the easiest thing to do is to close the socket and re-opening it prior to sending the second message, the REQREP mechanism would then make sure that the correct response is received without the need for such a sophisticated mechanism. From a performance perspective, it is slower to close and reopen a socket but in the workflow I have in mind where you need to send two messages in a row, this is usually in response to a rare event (such as not getting your first message answer in due time), so the performance impact would be negligeable. So maybe obsoleting/removing these options would be a good thing (do you have a way of asking the community about it?).

Problem: Since pull request zeromq#1730 was merged, protocol for REQ socket is
checked at the session level and this check does not take into account
the possibility of a request_id being part of the message. Thus the option
ZMQ_REQ_CORRELATE would no longer work.
This is now fixed: the possiblity of a 4 bytes integer being present
before the delimiter frame is taken into account (whether or not this
breaks the REQ/REP RFC is another issue).
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 74.669% when pulling 625b618 on FredTreg:master into 0feec7a on zeromq:master.

@c-rack c-rack merged commit 7a563eb into zeromq:master Apr 2, 2016
@bluca
Copy link
Member

bluca commented Aug 31, 2019

@FredTreg could you please send a new pull request or an email with a relicense statement? As explained in https://github.com/zeromq/libzmq/blob/master/.github/CONTRIBUTING.md and https://github.com/zeromq/libzmq/tree/master/RELICENSE we are trying to relicense libzmq under MPL-2. Thank you!

@bluca
Copy link
Member

bluca commented Feb 3, 2020

@FredTreg another ping. Please consider helping us with the relicense. Thank you!

@FredTreg
Copy link
Contributor Author

FredTreg commented Feb 3, 2020 via email

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.

4 participants