Fix #496 blind event clients after dev restart (attempt 2)

[mliszcz]

This PR restores Attribute::client_lib contents from temporary storage during DevReset handling.

Per original issue, attribute shall be identified by its name instead of index in multiattribute. This change is included as well but I was unable to reproduce this issue in a test (dynamic attribute created with IOAddAttribute was somehow persisted during the reset).

Fixes #496.

[t-b]

Review:

The PR needs rebasing. Not sure why github does not see that. This will then also fix cases where std namespace is still missing.

6f4893a (Use Attribute methods to update subscription times, 2019-07-20)

Very nice!

e47e2a9 (Rename Attribute::client_lib and add type alias, 2019-07-20)

My original comments are implemented in the next commit. So I'll spare them ;)

60866ee (Rename Attribute's event client_lib access methods, 2019-07-20)

--- a/cppapi/server/eventcmds.cpp
+++ b/cppapi/server/eventcmds.cpp
@@ -527,9 +527,12 @@ void DServer::event_subscription(string &dev_name,string &obj_name,string &actio
 //
 
                if (client_lib != 0)
-        {
-            omni_mutex_lock oml(EventSupplier::get_event_mutex());
-                       attribute.set_client_lib(client_lib,event);
+               {
+                       EventType event_type = PERIODIC_EVENT;
+                       tg->event_name_2_event_type(event, event_type);
+
+                       omni_mutex_lock oml(EventSupplier::get_event_mutex());
+                       attribute.add_event_client_lib_version(client_lib, event_type);
         }
        }
        else if (event == EventName[PIPE_EVENT])

I think it is difficult to grasp here that the only reason why you intialize event_type is to avoid a compiler warning.

How about adding a overload of event_name_2_event_type which returns something? A la

inline void Util::event_name_2_event_type(std::string &event_name, EventType &et)
{
  et = event_name_2_event_type(event_name);
}

inline EventType Util::event_name_2_event_type(const std::string &event_name)
{
	if (event_name == "change")
		return CHANGE_EVENT;
	else if (event_name == "quality")
		return QUALITY_EVENT;
	else if (event_name == "periodic")
		return PERIODIC_EVENT;
	else if (event_name == "archive")
		return ARCHIVE_EVENT;
	else if (event_name == "user_event")
		return USER_EVENT;
	else if (event_name == "attr_conf" || event_name == "attr_conf_5")
		return ATTR_CONF_EVENT;
	else
		return DATA_READY_EVENT;
}

46c64bf (Replace Attribute's client_lib vector with set, 2019-07-20)

✨

6b51943 (Extract EventPar and rename to EventSubscriptionState, 2019-07-20)

+1

cea2196 (Extract interface change event from attribute events, 2019-07-20)

That is particularly difficult to understand. It would be nice to get a more descriptive commmit message.

And for changes like

-                       if (eve.size() != 0)
-                       {
-                               _map.insert(make_pair(dev_list[j]->get_name(),eve));
-                       }
+                       _map.insert(make_pair(dev_list[j]->get_name(),eve));

I think it would be nice to give a justification why this is faithful as you dropped the non-empty check.

47c292e (Rename fields in AttributeEventSubscriptionState, 2019-07-20)

Looks good, except

@@ -1604,9 +1597,7 @@ void MultiAttribute::set_event_param(const AttributeEventSubscriptionStates& eve
 {
        for (size_t i = 0;i < eve.size();i++)
        {
-               if (eve[i].attr_id != -1)
-               {
-                       Tango::Attribute &att = get_attr_by_ind(eve[i].attr_id);
+                       Tango::Attribute &att = get_attr_by_ind(eve[i].attribute_id);
 
                        {

which I suspect should be part of the last commit as attr_id == -1 can not be present anymore?

e6c25b1 (Move event subscription state access to Attribute, 2019-07-20)

Good!

88c1c71 (Rename client lib version fields in events state, 2019-07-20)

Looks good.

Now that I've read all of the PR, I unfortunately did not understand where the functional change no happened. Could you explain that in the relevant commits?

Thanks!

[t-b]

See comment

[mliszcz]

@t-b thanks a lot for a very detailed review!

But your comments apply to another PR. For #496 there were two PRs:

• Fix #496 blind event clients after dev restart (attempt 2) #573 (Fix Client subscribing to events can be blind for up to 200 seconds if the target device is restarted (device restart) #496 blind event clients after dev restart (attempt 2)) -this one
• Fix #496 blind event clients after dev restart #572 (Fix Client subscribing to events can be blind for up to 200 seconds if the target device is restarted (device restart) #496 blind event clients after dev restart) - closed long ago in favor of this one.

In #572 I wanted to do some general refactoring (mostly renaming and fixing encapsulation issues) in client subscription memorization code before correcting the issue. But I never really finished that PR and gave up.

Maybe in the future we could consider rebasing and merging changes from #572 (and your comments are really helpful to improve #572 quality), but for now I propose to merge #573 (which is simpler and fixes the issue). Is it ok for you?

[t-b]

@t-b Yes, this is definitly okay for me. And sorry for the mess-up.

[t-b]

Looks good. Now that I know the code better ;)

[mliszcz]

@bourtemb would it be possible for you to take a look at this PR? It is blocking #631 as well.

[Ingvord]

this struct name is unclear to me, is it typo? Should it be EventPair? Or is it something completely different? Anyway it says nothing from its name concerning its purpose etc

[mliszcz]

Thanks Igor!. Some time ago I started to refactor this area but dropped the PR as it grew too big. Here is the relevant commit that re-names EventPar to AttributeEventSubscriptionState.
6b51943

Do you want me to do the rename in this PR?

[Ingvord]

AttributeEventSubscriptionState sounds much more better! Yes, please rename. Thanks!

[mliszcz]

I'v re-based the PR on tango-9-lts and re-named EventPar to EventSubscriptionState (without Attribute prefix as there is still a flag for device interface chagne event. Splitting this class requires further refactoring).

[bourtemb]

Travis tests are currently broken on this PR (problem when compiling tango_admin).

It looks like the new include file event_subscription_state.h is not installed.

$ .travis/install_tango_admin.sh

Build tango_admin

-- The C compiler identification is GNU 8.3.0

-- The CXX compiler identification is GNU 8.3.0

-- Check for working C compiler: /usr/bin/cc

-- Check for working C compiler: /usr/bin/cc -- works

-- Detecting C compiler ABI info

-- Detecting C compiler ABI info - done

-- Detecting C compile features

-- Detecting C compile features - done

-- Check for working CXX compiler: /usr/bin/c++

-- Check for working CXX compiler: /usr/bin/c++ -- works

-- Detecting CXX compiler ABI info

-- Detecting CXX compiler ABI info - done

-- Detecting CXX compile features

-- Detecting CXX compile features - done

-- Found PkgConfig: /usr/bin/pkg-config (found version "0.29") 

-- Checking for one of the modules 'tango'

-- Configuring done

-- Generating done

-- Build files have been written to: /home/tango/tango_admin/build

make: Entering directory '/home/tango/tango_admin/build'

make[1]: Entering directory '/home/tango/tango_admin/build'

make[2]: Entering directory '/home/tango/tango_admin/build'

Scanning dependencies of target tango_admin

make[2]: Leaving directory '/home/tango/tango_admin/build'

make[2]: Entering directory '/home/tango/tango_admin/build'

[ 33%] Building CXX object CMakeFiles/tango_admin.dir/tango_admin.cpp.o

In file included from /usr/local/include/tango/device.h:43,

                 from /usr/local/include/tango/tango.h:124,

                 from /home/tango/tango_admin/tango_admin.cpp:54:

/usr/local/include/tango/multiattribute.h:41:10: fatal error: event_subscription_state.h: No such file or directory

 #include "event_subscription_state.h"

          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

compilation terminated.

[mliszcz]

Thanks @bourtemb! I've listed the new header in CMake configuration. BTW it would be nice if cmake could automatically pick up such headers.

[bourtemb]

Great work @mliszcz! Thanks!
It seems to work fine with archive, change and periodic events as well as for attribute configuration events and data ready events.
I noticed only one issue with the Pipe events after calling the RestartServer admin device command.
Pipe events seem to work fine after a device restart command.

[mliszcz]

@bourtemb There is special handling for Pipe events in this area. There was no flag for Pipe events in EventPar. I need to take a closer look. Thanks!

[mliszcz]

As decided in today's Kernel meeting we will not fix Pipe event restoring in this PR (as main goal here is to track subscriptions by attribute name instead of attribute id).

I'll open a new issue to track the interaction of Pipe events and Device restarts. Probably a pipe-related flag should go there:

struct EventSubscriptionState
{
    std::string attribute_name;

    EventClientLibVersions change;
    EventClientLibVersions archive;
    EventClientLibVersions periodic;
    EventClientLibVersions user;
    EventClientLibVersions att_conf;

    bool quality;
    bool data_ready;
    bool dev_intr_change;

    bool notifd;
    bool zmq;
};

[bourtemb]

@mliszcz Please note I've updated my comment. There was a mistake in my comment. I was wrongly mentioning an issue with DevRestart admin device Command. I wanted to write ServerRestart admin device command:

I noticed only one issue with the Pipe events after calling the RestartServer admin device command.
Pipe events seem to work fine after a device restart command.

[bourtemb]

Looks good to me!
Thanks @mliszcz and thanks to the reviewers. This seems to fix most of the problems. Great work!
As agreed during the Tango Kernel teleconf, the remaining, the fix for the Pipe events after RestartServer command will be handled in another PR.

[mliszcz]

@Ingvord and @t-b, could you please have a look one more time? Rebasing and renaming (as Igor proposed) dismissed your reviews.