From 0af852dae0f4e2e5cd486304272f4d6db6d18af0 Mon Sep 17 00:00:00 2001 From: mliszcz Date: Thu, 23 Jan 2020 16:05:32 +0100 Subject: [PATCH 1/3] Protect storing last attribute value with mutex Fixes data race between: * polling thread (EventSupplier::detect_and_push_xxx_event) * and user thread pushing events (Attribute::fire_xxx_event) This is a backport of 5251bee from tango-9-lts. --- cppapi/server/attribute.cpp | 122 +++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 57 deletions(-) diff --git a/cppapi/server/attribute.cpp b/cppapi/server/attribute.cpp index acb74b890..61621c9ef 100644 --- a/cppapi/server/attribute.cpp +++ b/cppapi/server/attribute.cpp @@ -4017,54 +4017,58 @@ void Attribute::fire_change_event(DevFailed *except) bool force_change = false; bool quality_change = false; - if ((except != NULL) || - (quality == Tango::ATTR_INVALID) || - ((except == NULL) && (prev_change_event.err == true)) || - ((quality != Tango::ATTR_INVALID) && - (prev_change_event.quality == Tango::ATTR_INVALID))) - { - force_change = true; - } - vector filterable_names; vector filterable_data; vector filterable_names_lg; vector filterable_data_lg; - if (except != NULL) - { - prev_change_event.err = true; - prev_change_event.except = *except; - } - else { - Tango::AttrQuality the_quality; + omni_mutex_lock oml(EventSupplier::get_event_mutex()); - if (send_attr_5 != NULL) + if ((except != NULL) || + (quality == Tango::ATTR_INVALID) || + ((except == NULL) && (prev_change_event.err == true)) || + ((quality != Tango::ATTR_INVALID) && + (prev_change_event.quality == Tango::ATTR_INVALID))) { - the_quality = send_attr_5->quality; - prev_change_event.value_4 = send_attr_5->value; + force_change = true; } - else if (send_attr_4 != NULL) + + if (except != NULL) { - the_quality = send_attr_4->quality; - prev_change_event.value_4 = send_attr_4->value; + prev_change_event.err = true; + prev_change_event.except = *except; } else { - the_quality = send_attr->quality; - prev_change_event.value = send_attr->value; - } + Tango::AttrQuality the_quality; - if (prev_change_event.quality != the_quality) - { - quality_change = true; - } + if (send_attr_5 != NULL) + { + the_quality = send_attr_5->quality; + prev_change_event.value_4 = send_attr_5->value; + } + else if (send_attr_4 != NULL) + { + the_quality = send_attr_4->quality; + prev_change_event.value_4 = send_attr_4->value; + } + else + { + the_quality = send_attr->quality; + prev_change_event.value = send_attr->value; + } + + if (prev_change_event.quality != the_quality) + { + quality_change = true; + } - prev_change_event.quality = the_quality; - prev_change_event.err = false; + prev_change_event.quality = the_quality; + prev_change_event.err = false; + } + prev_change_event.inited = true; } - prev_change_event.inited = true; filterable_names.push_back("forced_event"); if (force_change == true) @@ -4478,40 +4482,44 @@ void Attribute::fire_archive_event(DevFailed *except) vector filterable_names_lg; vector filterable_data_lg; - if (except != NULL) { - prev_archive_event.err = true; - prev_archive_event.except = *except; - } - else - { - Tango::AttrQuality the_quality; + omni_mutex_lock oml(EventSupplier::get_event_mutex()); - if (send_attr_5 != Tango_nullptr) - { - prev_archive_event.value_4 = send_attr_5->value; - the_quality = send_attr_5->quality; - } - else if (send_attr_4 != Tango_nullptr) + if (except != NULL) { - prev_archive_event.value_4 = send_attr_4->value; - the_quality = send_attr_4->quality; + prev_archive_event.err = true; + prev_archive_event.except = *except; } else { - prev_archive_event.value = send_attr->value; - the_quality = send_attr->quality; - } + Tango::AttrQuality the_quality; - if (prev_archive_event.quality != the_quality) - { - quality_change = true; - } + if (send_attr_5 != Tango_nullptr) + { + prev_archive_event.value_4 = send_attr_5->value; + the_quality = send_attr_5->quality; + } + else if (send_attr_4 != Tango_nullptr) + { + prev_archive_event.value_4 = send_attr_4->value; + the_quality = send_attr_4->quality; + } + else + { + prev_archive_event.value = send_attr->value; + the_quality = send_attr->quality; + } - prev_archive_event.quality = the_quality; - prev_archive_event.err = false; + if (prev_archive_event.quality != the_quality) + { + quality_change = true; + } + + prev_archive_event.quality = the_quality; + prev_archive_event.err = false; + } + prev_archive_event.inited = true; } - prev_archive_event.inited = true; filterable_names.push_back("forced_event"); if (force_change == true) From 82f1b8916a03c5f1f0604ec3fe5438e2630e0da5 Mon Sep 17 00:00:00 2001 From: mliszcz Date: Thu, 23 Jan 2020 16:13:45 +0100 Subject: [PATCH 2/3] Change mutex used for EventSupplier::detect_change Remove detect_mutex and protect detect_change method with event_mutex to synchronize access to the old attribute value. Fixes data race between: * polling thread (EventSupplier::detect_and_push_xxx_event) * and user thread pushing events (EventSupplier::detect_change) This is a backport of 8d4a1bb from tango-9-lts. --- cppapi/server/attribute.cpp | 35 +++++++++++++++++++++------------ cppapi/server/eventsupplier.cpp | 6 ------ cppapi/server/eventsupplier.h | 4 +--- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/cppapi/server/attribute.cpp b/cppapi/server/attribute.cpp index 61621c9ef..f73074eed 100644 --- a/cppapi/server/attribute.cpp +++ b/cppapi/server/attribute.cpp @@ -4462,8 +4462,6 @@ void Attribute::fire_archive_event(DevFailed *except) { // -// Execute detect_change only to calculate the delta_change_rel and -// delta_change_abs and force_change ! // bool force_change = false; @@ -4471,20 +4469,26 @@ void Attribute::fire_archive_event(DevFailed *except) double delta_change_rel = 0.0; double delta_change_abs = 0.0; - if (event_supplier_nd != NULL) - event_supplier_nd->detect_change(*this, ad,true,delta_change_rel,delta_change_abs,except,force_change,dev); - else if (event_supplier_zmq != NULL) - event_supplier_zmq->detect_change(*this, ad,true,delta_change_rel,delta_change_abs,except,force_change,dev); - - - vector filterable_names; - vector filterable_data; - vector filterable_names_lg; - vector filterable_data_lg; - { omni_mutex_lock oml(EventSupplier::get_event_mutex()); + // Execute detect_change only to calculate the delta_change_rel and + // delta_change_abs and force_change ! + + if (event_supplier_nd || event_supplier_zmq) + { + EventSupplier* event_supplier = event_supplier_nd ? event_supplier_nd : event_supplier_zmq; + event_supplier->detect_change( + *this, + ad, + true, + delta_change_rel, + delta_change_abs, + except, + force_change, + dev); + } + if (except != NULL) { prev_archive_event.err = true; @@ -4521,6 +4525,11 @@ void Attribute::fire_archive_event(DevFailed *except) prev_archive_event.inited = true; } + vector filterable_names; + vector filterable_data; + vector filterable_names_lg; + vector filterable_data_lg; + filterable_names.push_back("forced_event"); if (force_change == true) filterable_data.push_back((double)1.0); diff --git a/cppapi/server/eventsupplier.cpp b/cppapi/server/eventsupplier.cpp index 0c7409226..c6732f740 100644 --- a/cppapi/server/eventsupplier.cpp +++ b/cppapi/server/eventsupplier.cpp @@ -1182,12 +1182,6 @@ bool EventSupplier::detect_change(Attribute &attr, struct SuppliedEventData &att the_new_any = &(attr_value.attr_val->value); } -// -// get the mutex to synchronize the sending of events -// - - omni_mutex_lock l(detect_mutex); - // // Send event, if the read_attribute failed or if it is the first time that the read_attribute succeed after a failure. // Same thing if the attribute quality factor changes to INVALID diff --git a/cppapi/server/eventsupplier.h b/cppapi/server/eventsupplier.h index 701f374fc..fc245153b 100644 --- a/cppapi/server/eventsupplier.h +++ b/cppapi/server/eventsupplier.h @@ -174,9 +174,7 @@ protected : static omni_mutex push_mutex; static omni_condition push_cond; - // Added a mutex to synchronize the access to - // detect_event which is used - // from different threads + // Unused, replaced with event_mutex. Left for backward compatibility. static omni_mutex detect_mutex; private: From 5f8956603221e5f651982565f4027efebb1028eb Mon Sep 17 00:00:00 2001 From: mliszcz Date: Mon, 16 Dec 2019 14:46:53 +0100 Subject: [PATCH 3/3] Protect event subscription timestamps with mutex Fixes data race between: * omni worker thread (DServer::event_subscription) * and user thread pushing events (Attribute::fire_xxx_event) This is a backport of ee262d4 from tango-9-lts. --- cppapi/server/attribute.cpp | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/cppapi/server/attribute.cpp b/cppapi/server/attribute.cpp index f73074eed..ea5216af7 100644 --- a/cppapi/server/attribute.cpp +++ b/cppapi/server/attribute.cpp @@ -3774,9 +3774,13 @@ void Attribute::fire_change_event(DevFailed *except) time_t change3_subscription,change4_subscription,change5_subscription; now = time(NULL); - change3_subscription = now - event_change3_subscription; - change4_subscription = now - event_change4_subscription; - change5_subscription = now - event_change5_subscription; + + { + omni_mutex_lock oml(EventSupplier::get_event_mutex()); + change3_subscription = now - event_change3_subscription; + change4_subscription = now - event_change4_subscription; + change5_subscription = now - event_change5_subscription; + } // // Get the event supplier(s) @@ -4203,9 +4207,12 @@ void Attribute::fire_archive_event(DevFailed *except) now = time(NULL); - archive3_subscription = now - event_archive3_subscription; - archive4_subscription = now - event_archive4_subscription; - archive5_subscription = now - event_archive5_subscription; + { + omni_mutex_lock oml(EventSupplier::get_event_mutex()); + archive3_subscription = now - event_archive3_subscription; + archive4_subscription = now - event_archive4_subscription; + archive5_subscription = now - event_archive5_subscription; + } // // Get the event supplier(s) @@ -4659,9 +4666,12 @@ void Attribute::fire_event(vector &filt_names,vector &filt_vals, now = time(NULL); - user3_subscription = now - event_user3_subscription; - user4_subscription = now - event_user4_subscription; - user5_subscription = now - event_user5_subscription; + { + omni_mutex_lock oml(EventSupplier::get_event_mutex()); + user3_subscription = now - event_user3_subscription; + user4_subscription = now - event_user4_subscription; + user5_subscription = now - event_user5_subscription; + } // // Get the event supplier(s) @@ -4931,9 +4941,12 @@ void Attribute::fire_error_periodic_event(DevFailed *except) now = time(NULL); - periodic3_subscription = now - event_periodic3_subscription; - periodic4_subscription = now - event_periodic4_subscription; - periodic5_subscription = now - event_periodic5_subscription; + { + omni_mutex_lock oml(EventSupplier::get_event_mutex()); + periodic3_subscription = now - event_periodic3_subscription; + periodic4_subscription = now - event_periodic4_subscription; + periodic5_subscription = now - event_periodic5_subscription; + } vector client_libs = get_client_lib(PERIODIC_EVENT); // We want a copy