problem: request socket state machine check is not running

[somdoron]

following commit removed the virtual modifier from push_msg/pull_msg from session base, causing the req_session state machine that validated the REQ-REP rfc not to run.

This PR fixes this problem by returning the virtual modifier.

However because of this fix it break the ZMQ_REQ_CORRELATE feature.
ZMQ_REQ_CORRELATE is breaking the REQ-REP rfc.

Bottom line, we either fix the ZMQ_REQ_CORRELATE feature (and document it in the RFC) or should remove those features.

[FredTreg]

Hi,
Independently of whether the rfc is broken or not, I see two ways of fixing the ZMQ_REQ_CORRELATE option:

Option 1
Remove completely the req_session_t class and use the session_base_t class for the ZMQ_REQ socket. Indeed, the req_session_t seems to do some over checking: why would we keep a class which only purpose seems to check validaty of the code that was produced a few lines above. The only thing that this class does is to check that the REQ frames are pushed in the correct order in its push_msgmethod. Unless I missed something, these frames are only produced by the req_t class and unless there is a bug in the code, the frames can only be sent in the correct order.

So this class seems to act only as a big assert and should be removed as any check it does would be checked by the unit tests anyway.

Also, the only two other socket types subclassing session_base_t are ZMQ_RADIO and ZMQ_DISH but these types do some actual processing on their push_msg method. This means that for example the ZMQ_REP does not implement checks similar to the req_session_twhen it could: the REP socket could also check that frames are sent in the correct order but it does not. So the ZMQ_REQ subclassing session_base_t seems to be an oddity that could be removed with no harm.

Option 2
Fix the req_session_tclass and take into account the possibility of the ZMQ_REQ_CORRELATE option being turned on.
I have the code ready to be pulled for option 2.

What do you think?

[somdoron]

I think option 2. You missed something important with option 1, unfriendly
peer can dilbertly try and sabotage, the req session protecting that. Now
you can do that in application code but that will be breaking change.
On Mar 26, 2016 12:09, "Frederic Tregon" notifications@github.com wrote:

Hi,
Independently of whether the rfc is broken or not, I see two ways of
fixing the ZMQ_REQ_CORRELATE option:

Option 1
Remove completely the req_session_t class and use the session_base_t
class for the ZMQ_REQ socket. Indeed, the req_session_t seems to do some
over checking: why would we keep a class which only purpose seems to check
validaty of the code that was produced a few lines above. The only thing
that this class does is to check that the REQ frames are pushed in the
correct order in its push_msgmethod. Unless I missed something, these
frames are only produced by the req_t class and unless there is a bug in
the code, the frames can only be sent in the correct order.

So this class seems to act only as a big assert and should be removed as
any check it does would be checked by the unit tests anyway.

Also, the only two other socket types subclassing session_base_t are
ZMQ_RADIO and ZMQ_DISH but these types do some actual processing on their
push_msg method. This means that for example the ZMQ_REP does not
implement checks similar to the req_session_twhen it could: the REP
socket could also check that frames are sent in the correct order but it
does not. So the ZMQ_REQ subclassing session_base_t seems to be an oddity
that could be removed with no harm.

Option 2
Fix the req_session_tclass and take into account the possibility of the
ZMQ_REQ_CORRELATE option being turned on.
I have the code ready to be pulled for option 2.

What do you think?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1730 (comment)

[FredTreg]

Thanks for taking the time to answer.

OK. So first I misunderstood the role of push_msg, I though it was used for sending messages to peers, obviously it is for receiving messages from peers (sorry I got confused, the push/pull wording really depends on the perspective :)

Then, I still have two questions:

1. Why is the similar check not done for the REP socket. It too could receive unfriendly messages (for example without the delimiter frame).
2. Why would we check this at the session level (in method push_msg) rather than at the socket level (in method xrecv)? In fact, this is already the case for both the REQ and REP sockets: they both check in their xrecv methods that the messages protocol is correct. The only difference I see is that these checks silently ignore errors and wait for the next valid message whereas the push_msgmethod will fail (and I guess will probably close the peer).

[somdoron]

1. you are right, it should
2. The reason is, I think, it to close peers that don't use the protocol.