Problem: poller_t invalid behavior on moved sockets #208
Conversation
On adding invalid socket (e.g. after move) there was exception thrown but leaving modified and unconsistent internal state. Besides that there was no possibility to remove a socket that was moved into. Solutions: check for socket validity (added operator bool) and changed internal unordered_map "handlers" to operator on zmq internal pointers. Added two test cases covering the issues.
Pull Request Test Coverage Report for Build 38
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 38
💛 - Coveralls |
Pull Request Test Coverage Report for Build 39
💛 - Coveralls |
zmq.hpp
Outdated
| @@ -1053,10 +1058,12 @@ namespace zmq | |||
|
|
|||
| void add (zmq::socket_t &socket, short events, handler_t handler) | |||
| { | |||
| if (!socket) | |||
| throw error_t (); | |||
There was a problem hiding this comment.
errno must be set to EINVAL for consistency, or error_t must be created with an explicit error code of EINVAL. An appropriate ctor of error_t must be added.
There was a problem hiding this comment.
Thank you for review. I don't think that setting EINVAL in cpp wrapper is a good idea, we have exceptions already for carrying information and should not interfere with libzmq errno but I get the point that someone might rely on errno after throw and pattern is we throw after zmq_ call. Updated a review to throw only after zmq call fails so passing handling of nullptr socket to zmq in this case. Thanks.
There was a problem hiding this comment.
The ctor of error_t reads errno. It is so far only intended to be thrown when there was a libzmq error.
As it is now, the exception will contain an unpredictable errno, whatever value errno had before.
There was a problem hiding this comment.
Have you seen updated pull request?
There was a problem hiding this comment.
I now saw it... Sorry I did not read your comment properly earlier. Your change solves the problem I mentioned, but I think it has another problem.
| auto inserted = false; | ||
| if (handler) | ||
| std::tie(it, inserted) = handlers.emplace (socket.ptr, std::move (handler)); | ||
| if (0 == zmq_poller_add (poller_ptr, socket.ptr, inserted ? &(it->second) : nullptr, events)) { |
There was a problem hiding this comment.
I don't think this is a sensible behaviour, if inserted == false. This would mean that add had already been called for the same socket, which is not a valid use. There probably should be a test for this case.
There was a problem hiding this comment.
Maybe this can be addressed later.
There was a problem hiding this comment.
I think it is a valid use case and you are right that that if inserted is false it means socket already added but it will behave properly: rollback would not be triggered, only exception with errno specific to "already present" scenario. And this case is already covered by test add_handler_twice_throws.
There was a problem hiding this comment.
Ok, fine then :) Sorry I missed that a test for this already exists.
| if (0 == zmq_poller_add (poller_ptr, socket.ptr, handler_ptr, events)) { | ||
| auto it = std::end (handlers); | ||
| auto inserted = false; | ||
| if (handler) |
There was a problem hiding this comment.
The comment was removed, but it is still applicable.
There was a problem hiding this comment.
Sorry, should have explained that. This comment was only confusing as it is valid to pass nullptr user data to zmq_poller_add. libzmq does not care what user passes there, it only passively handles it back on wait. Similarly to actors in czmq
There was a problem hiding this comment.
It is valid for libzmq/zmq_poller_add, but it is not sensible in the way cppzmq uses it. In wait, a no-op results if handler was nullptr here.
There was a problem hiding this comment.
if you are referring to if we should allow empty handler at all then this is a separate discussion.
You should not create //todo comments for that because that might indicate that something was agreed (where?) and waiting to be done. If you think if something can be improved why not create an issue open for discussion, or provide pull request..
In my opinion we should allow empty handlers, it is a valid use case where user is interested in wait signalling only by returning and no-op but I am open for discussion.
There was a problem hiding this comment.
My original comment was indeed mistaken. When I wrote it, I thought that it would lead to some error later on, but this is not the case. So I am fine to remove it.
If you used a poller with a single socket and an empty handler, it might make sense. But it is somewhat misaligned with the design of poller_t, which manages and calls back into the handlers. I would still tend to disallow is, but I am also fine to leave it as it is. It would be good to have a test that uses it this way (i.e. not only for adding an empty handler, but also polling afterwards).
On adding invalid socket (e.g. after move) there was exception thrown
but leaving modified and unconsistent internal state.
Besides that there was no possibility to remove a socket that was moved
into.
Solutions: changed internal unordered_map "handlers" to operator on zmq internal pointers and rollback when zmq_poller_add fails.
Added two test cases covering the issues.