Skip to content
This repository has been archived by the owner on Jul 8, 2022. It is now read-only.

Fix issue where unsubscribing in push_events led to API_EventTimeout … #699

Merged
merged 2 commits into from
Apr 23, 2020
Merged

Fix issue where unsubscribing in push_events led to API_EventTimeout … #699

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
154b32c
Fix issue where unsubscribing in push_events led to API_EventTimeout …
bourtemb Mar 25, 2020
a5d2c80
Stop delaying events only if there is no other ZMQ_DELAY_EVENT comman…
bourtemb Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions cppapi/client/zmqeventconsumer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,9 @@ bool ZmqEventConsumer::process_ctrl(zmq::message_t &received_ctrl,zmq::pollitem_
{
bool ret = false;

// static variable to count the number of ZMQ_DELAY_EVENT requests currently in progress:
static int nb_current_delay_event_requests = 0;

//
// For debug and logging purposes
//
Expand Down Expand Up @@ -1075,14 +1078,32 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is exactly what I had in mind.

// and we are currently only taking care of messages received on the control socket
// No need to update old_poll_nb in this case because it is already correct
// otherwise this would lead to issues like https://github.com/tango-controls/cppTango/issues/686
// where events would no longer be received if someone subscribes or unsubscribes to events in
// an event callback and when the callback is executed during a subscribe_event call
if (poll_nb != 1)
{
old_poll_nb = poll_nb;
poll_nb = 1;
Comment on lines +1089 to +1090
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to do something like that for tango >= 9.4 (in tango-9-lts branch) because this implies dropping support for multicast events.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out! I was not aware that this will impact multicast events (I am not familiar with how those are implemented).

}
nb_current_delay_event_requests++;
}
break;

case ZMQ_RELEASE_EVENT:
{
poll_nb = old_poll_nb;
if(nb_current_delay_event_requests >= 1)
{
nb_current_delay_event_requests--;
}
if(nb_current_delay_event_requests == 0)
{
// Stop delaying events only if there is no other ZMQ_DELAY_EVENT command requested
poll_nb = old_poll_nb;
}
}
break;

Expand Down