Fix issue where unsubscribing in push_events led to API_EventTimeout … #699
Conversation
| old_poll_nb = poll_nb; | ||
| poll_nb = 1; |
There was a problem hiding this comment.
Can these two variables be completely removed and replaced with just one boolean flag (e.g. this->delay_events = true;)?
Then it could be possible to change zmq::poll(items,nb_poll_item,-1); into:
const int ALL_SOCKETS = 3;
const int ONLY_CONTROL_SOCKET = 1;
zmq::poll(items, delay_events ? ONLY_CONTROL_SOCKET : ALL_SOCKETS, -1)I think this bug would never appear in this case.
There was a problem hiding this comment.
I propose to do something like that for tango >= 9.4 (in tango-9-lts branch) because this implies dropping support for multicast events.
There was a problem hiding this comment.
Thanks for pointing that out! I was not aware that this will impact multicast events (I am not familiar with how those are implemented).
| @@ -1075,8 +1075,17 @@ bool ZmqEventConsumer::process_ctrl(zmq::message_t &received_ctrl,zmq::pollitem_ | |||
|
|
|||
| case ZMQ_DELAY_EVENT: | |||
| { | |||
| old_poll_nb = poll_nb; | |||
| poll_nb = 1; | |||
| // If poll_nb == 1, then we are already in a situation where events are being delayed | |||
There was a problem hiding this comment.
In this double-delay scenario, shouldn't we count how many ZMQ_DELAY_EVENTs were received and wait for as many ZMQ_RELEASE_EVENTs?
Can we run into an issue when e.g. two threads create DelatEvent object?
There was a problem hiding this comment.
Good point! I agree with you it would make sense to count the number of received ZMQ_DELAY_EVENTS and really release once we have received as many ZMQ_RELEASE_EVENTS as ZMQ_DELAY_EVENTS.
There might be other issues if we don't do that
There was a problem hiding this comment.
@mliszcz , I implemented this counter in a5d2c80
In cppTango >= 9.4, we could eventually make this variable a private member of ZmqEventConsumer class.
I didn't do it here to preserve the binary compatibility.
For the moment, this variable is useful/used only in ZmqEventConsumer::process_ctrl() method and the ZMQEventConsumer object is a singleton so it should be fine like this.
Could you please have a look?
There was a problem hiding this comment.
Thanks! This is exactly what I had in mind.
…ango-controls#686) Subscribing or unsubscribing events in an event callback when this callback was called during a subscribe_event() phase was leading to events and heartbeat events no longer received by the event client. A bug was fixed when several ZMQ_DELAY_EVENT commands were received consecutively by the ZMQ control socket.
cd2f911 to
a5d2c80
Compare
|
Hi all, |
|
sardanatestsuite also works with this PR. Thanks a lot! |
…ngelog-for-699 Update CHANGELOG for #699
…(#686)
Subscribing or unsubscribing events in an event callback when this callback
was called during a subscribe_event() phase was leading to events and
heartbeat events no longer received by the event client.
A bug was fixed when several ZMQ_DELAY_EVENT tasks were received consecutively
by the ZMQ control so.