diff --git a/CHANGES b/CHANGES index f115e84..65ddba3 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,12 @@ Changes ======= +v3.1.12.6 +- Fix race condition leading to event delivery after unsubscription + +v3.1.12.5 +- Initialize valueType in copy ctors of Variant class + v3.1.12.4 - Fix calling of registered subscription status handlers for selective broadcasts diff --git a/include/CommonAPI/Event.hpp b/include/CommonAPI/Event.hpp index ab656f8..d2be59d 100644 --- a/include/CommonAPI/Event.hpp +++ b/include/CommonAPI/Event.hpp @@ -97,8 +97,8 @@ class Event { ListenersMap pendingSubscriptions_; SubscriptionsSet pendingUnsubscriptions_; - std::mutex notificationMutex_; - std::mutex subscriptionMutex_; + std::recursive_mutex mutex_; + std::mutex abi_placeholder_; }; template @@ -107,13 +107,14 @@ typename Event::Subscription Event::subscribe(List bool isFirstListener; Listeners listeners; - subscriptionMutex_.lock(); - subscription = nextSubscription_++; - isFirstListener = (0 == pendingSubscriptions_.size()) && (pendingUnsubscriptions_.size() == subscriptions_.size()); - listener = std::move(listener); - listeners = std::make_tuple(listener, std::move(errorListener)); - pendingSubscriptions_[subscription] = std::move(listeners); - subscriptionMutex_.unlock(); + { + std::lock_guard itsLock(mutex_); + subscription = nextSubscription_++; + isFirstListener = (0 == pendingSubscriptions_.size()) && (pendingUnsubscriptions_.size() == subscriptions_.size()); + listener = std::move(listener); + listeners = std::make_tuple(listener, std::move(errorListener)); + pendingSubscriptions_[subscription] = std::move(listeners); + } if (isFirstListener) { if (!pendingUnsubscriptions_.empty()) @@ -131,30 +132,31 @@ void Event::unsubscribe(const Subscription subscription) { bool hasUnsubscribed(false); Listener listener; - subscriptionMutex_.lock(); - auto listenerIterator = subscriptions_.find(subscription); - if (subscriptions_.end() != listenerIterator) { - if (pendingUnsubscriptions_.end() == pendingUnsubscriptions_.find(subscription)) { - if (0 == pendingSubscriptions_.erase(subscription)) { - pendingUnsubscriptions_.insert(subscription); - listener = std::get<0>(listenerIterator->second); - hasUnsubscribed = true; + { + std::lock_guard itsLock(mutex_); + auto listenerIterator = subscriptions_.find(subscription); + if (subscriptions_.end() != listenerIterator) { + if (pendingUnsubscriptions_.end() == pendingUnsubscriptions_.find(subscription)) { + if (0 == pendingSubscriptions_.erase(subscription)) { + pendingUnsubscriptions_.insert(subscription); + listener = std::get<0>(listenerIterator->second); + hasUnsubscribed = true; + } + isLastListener = (pendingUnsubscriptions_.size() == subscriptions_.size()); } - isLastListener = (pendingUnsubscriptions_.size() == subscriptions_.size()); } - } - else { - listenerIterator = pendingSubscriptions_.find(subscription); - if (pendingSubscriptions_.end() != listenerIterator) { - listener = std::get<0>(listenerIterator->second); - if (0 != pendingSubscriptions_.erase(subscription)) { - isLastListener = (pendingUnsubscriptions_.size() == subscriptions_.size()); - hasUnsubscribed = true; + else { + listenerIterator = pendingSubscriptions_.find(subscription); + if (pendingSubscriptions_.end() != listenerIterator) { + listener = std::get<0>(listenerIterator->second); + if (0 != pendingSubscriptions_.erase(subscription)) { + isLastListener = (pendingUnsubscriptions_.size() == subscriptions_.size()); + hasUnsubscribed = true; + } } } + isLastListener = isLastListener && (0 == pendingSubscriptions_.size()); } - isLastListener = isLastListener && (0 == pendingSubscriptions_.size()); - subscriptionMutex_.unlock(); if (hasUnsubscribed) { onListenerRemoved(listener, subscription); @@ -166,8 +168,7 @@ void Event::unsubscribe(const Subscription subscription) { template void Event::notifyListeners(const Arguments_&... eventArguments) { - subscriptionMutex_.lock(); - notificationMutex_.lock(); + std::lock_guard itsLock(mutex_); for (auto iterator = pendingUnsubscriptions_.begin(); iterator != pendingUnsubscriptions_.end(); iterator++) { @@ -182,18 +183,14 @@ void Event::notifyListeners(const Arguments_&... eventArguments) } pendingSubscriptions_.clear(); - subscriptionMutex_.unlock(); for (auto iterator = subscriptions_.begin(); iterator != subscriptions_.end(); iterator++) { (std::get<0>(iterator->second))(eventArguments...); } - - notificationMutex_.unlock(); } template void Event::notifySpecificListener(const Subscription subscription, const Arguments_&... eventArguments) { - subscriptionMutex_.lock(); - notificationMutex_.lock(); + std::lock_guard itsLock(mutex_); for (auto iterator = pendingUnsubscriptions_.begin(); iterator != pendingUnsubscriptions_.end(); iterator++) { @@ -209,22 +206,16 @@ void Event::notifySpecificListener(const Subscription subscriptio } pendingSubscriptions_.clear(); - - subscriptionMutex_.unlock(); for (auto iterator = subscriptions_.begin(); iterator != subscriptions_.end(); iterator++) { if (subscription == iterator->first) { (std::get<0>(iterator->second))(eventArguments...); } } - - notificationMutex_.unlock(); } template void Event::notifySpecificError(const Subscription subscription, const CallStatus status) { - - subscriptionMutex_.lock(); - notificationMutex_.lock(); + std::lock_guard itsLock(mutex_); for (auto iterator = pendingUnsubscriptions_.begin(); iterator != pendingUnsubscriptions_.end(); iterator++) { @@ -239,7 +230,6 @@ void Event::notifySpecificError(const Subscription subscription, } pendingSubscriptions_.clear(); - subscriptionMutex_.unlock(); for (auto iterator = subscriptions_.begin(); iterator != subscriptions_.end(); iterator++) { if (subscription == iterator->first) { ErrorListener listener = std::get<1>(iterator->second); @@ -249,10 +239,7 @@ void Event::notifySpecificError(const Subscription subscription, } } - notificationMutex_.unlock(); - if (status != CommonAPI::CallStatus::SUCCESS) { - subscriptionMutex_.lock(); auto listenerIterator = subscriptions_.find(subscription); if (subscriptions_.end() != listenerIterator) { if (pendingUnsubscriptions_.end() == pendingUnsubscriptions_.find(subscription)) { @@ -267,14 +254,12 @@ void Event::notifySpecificError(const Subscription subscription, pendingSubscriptions_.erase(subscription); } } - subscriptionMutex_.unlock(); } } template void Event::notifyErrorListeners(const CallStatus status) { - subscriptionMutex_.lock(); - notificationMutex_.lock(); + std::lock_guard itsLock(mutex_); for (auto iterator = pendingUnsubscriptions_.begin(); iterator != pendingUnsubscriptions_.end(); iterator++) { @@ -289,16 +274,12 @@ void Event::notifyErrorListeners(const CallStatus status) { } pendingSubscriptions_.clear(); - subscriptionMutex_.unlock(); - for (auto iterator = subscriptions_.begin(); iterator != subscriptions_.end(); iterator++) { ErrorListener listener = std::get<1>(iterator->second); if (listener) { listener(status); } } - - notificationMutex_.unlock(); } diff --git a/include/CommonAPI/Variant.hpp b/include/CommonAPI/Variant.hpp index db1cf7b..c6cd40f 100644 --- a/include/CommonAPI/Variant.hpp +++ b/include/CommonAPI/Variant.hpp @@ -619,7 +619,9 @@ Variant::Variant() } template -Variant::Variant(const Variant &_source) { +Variant::Variant(const Variant &_source) : + valueType_(_source.valueType_) +{ AssignmentVisitor visitor(*this, false); ApplyVoidVisitor< AssignmentVisitor , Variant, Types_... @@ -627,7 +629,8 @@ Variant::Variant(const Variant &_source) { } template -Variant::Variant(Variant &&_source) +Variant::Variant(Variant &&_source) : + valueType_(_source.valueType_) { AssignmentVisitor visitor(*this, false); ApplyVoidVisitor< @@ -688,7 +691,9 @@ template Variant::Variant(const Type_ &_value, typename std::enable_if::value>::type*, typename std::enable_if::value>::type*, - typename std::enable_if>::value>::type*) { + typename std::enable_if>::value>::type*) : + valueType_(0x0) +{ set::type>(_value, false); } @@ -697,7 +702,9 @@ template Variant::Variant(Type_ &&_value, typename std::enable_if::value>::type*, typename std::enable_if::value>::type*, -typename std::enable_if>::value>::type*) { +typename std::enable_if>::value>::type*) : + valueType_(0x0) +{ set::type>(std::move(_value), false); }