-
Notifications
You must be signed in to change notification settings - Fork 756
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
Feat/monitor poll #414
base: master
Are you sure you want to change the base?
Feat/monitor poll #414
Conversation
Additional information:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extensions, I think these will be really useful.
Please see my comments on individual issues.
Also, please remember to reformat all commits with clang-format
.
Most of the travis builds are failing right now, could you give them a look and fix them, please?
@@ -2224,6 +2224,53 @@ class monitor_t | |||
on_monitor_started(); | |||
} | |||
|
|||
#if ZMQ_VERSION_MAJOR >= 4 | |||
bool get_event(zmq_event_t& eventMsg, std::string& address, zmq::recv_flags flags = zmq::recv_flags::none) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the signature of this method. What about
std::optional<std::pair<zmq_event_t, std::string>> get_event(zmq::recv_flags flags = zmq::recv_flags::none)
(This requires C++17 this way, but we can also do this with the fallback to detail::trivial_optional
as done for other types)
This removes the ambiguity of whether the original value of eventMsg
and address
are used and in what cases they are modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm not very familiar with std::optional and will try to implement this way
if (zmq_errno() == ETERM || zmq_errno() == EAGAIN) | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this also returns false on ETERM
? There's no way to distinguish EAGAIN
and ETERM
this way, and this seems inconsistent with the behavior of zmq::socket_t::recv
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check https://github.com/zeromq/cppzmq/blob/master/zmq.hpp#L2274.
I just kept same behaviour implemented in check_event(). If this is not mandatory, I can just throw an exception and not use a std::optionnal as described above, but just std::pair<>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yes, that's right. It's inconsistent one way or the other, but I think the behaviour of zmq::socket_t::recv
is the appropriate one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what we should implement in this case? Maybe we shall throw an exception?
|
||
if (rc == -1) | ||
{ | ||
if (zmq_errno() == ETERM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why does this return false
rather than throwing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above
|
||
{ | ||
message_t msg; | ||
int rc = zmq_msg_recv(msg.handle(), _monitor_socket.handle(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this use _monitor_socket.recv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, but then it is not possible to use the same behaviour as the old check_event. Otherwise I will need to use a try catch statement
} | ||
|
||
message_t addrMsg; | ||
int rc = zmq_msg_recv(addrMsg.handle(), _monitor_socket.handle(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like above, can't this use _monitor_socket.recv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See prev comment
Thanks for the extensions, I think these will be really useful.
Please see my comments on individual issues.
Also, please remember to reformat all commits with
clang-format
.
Could you add this to the contribution guidelines of the project?
Most of the travis builds are failing right now, could you give them a look and fix them, please?
I don't like to put effort on something that will be not merged. I was waiting for a green thumb before putting more work on this, as I did not asked anyone about the design of this PR ;)
Thanks for reviewing, will try to solve all of those next week. BR
{ monitor.handle(), 0, ZMQ_POLLIN, 0 }, | ||
}; | ||
|
||
zmq::poll(&items[0], 1, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the overload taking std::chrono::milliseconds
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger that
operator void *() ZMQ_NOTHROW { return handle(); } | ||
|
||
operator void const *() const ZMQ_NOTHROW { return handle(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not add these operators. socket_t
and context_t
have them because they are direct correspondents of the underlying C data structures. But that's not the case here, and it's not obvious that a cast to void*
returns the monitor socket handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
ZMQ_NODISCARD void *handle() ZMQ_NOTHROW { return _monitor_socket.handle(); } | ||
|
||
ZMQ_NODISCARD const void *handle() const ZMQ_NOTHROW { return _monitor_socket.handle(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment above, I'd use more specific names for these functions. We are in monitor_t
, so monitor
is probably redundant, but i'd use socket_handle
.
Also, the const
overload should be removed. There are no functions in the C zmq API that accept a const void*
socket. I now they exist for other classes, but that's rather a legacy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
@@ -2232,6 +2232,8 @@ class monitor_t | |||
|
|||
ZMQ_NODISCARD const void *handle() const ZMQ_NOTHROW { return _monitor_socket.handle(); } | |||
|
|||
operator socket_ref() ZMQ_NOTHROW { return (zmq::socket_ref) _monitor_socket; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I'd not add an operator here but rather a named socket()
member function. Actually, then the handle
/socket_handle
function is not necessary at all, since its handle can be accessed through the socket_ref
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
@@ -2414,6 +2414,15 @@ class monitor_t | |||
_socket = socket_ref(); | |||
} | |||
#endif | |||
|
|||
void close() ZMQ_NOTHROW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be done.
Any update on this? Is there anything you need to address the comments? Sorry I didn't get back until now, but from reading your responses I assumed that most things are clear and undisputed. |
Hi, Thanks for pinging back. I'm struggling to find some time to fix those issues. I will try to do my best in the next couple of weeks. Cheers |
This MR improve monitor_t class by addressing #381 and #109.
This lead to better application design and allows the same thread to do both monitoring and retrieving messages from the monitored socket, thus avoiding clunky code with threads joining everywhere.