From 88a84ab26aecb7f7cb8b4556b02e057b32c3e0f8 Mon Sep 17 00:00:00 2001 From: coderhyme Date: Mon, 3 Aug 2015 18:40:03 +0900 Subject: [PATCH] Fix deadlock issue from RCSResourceObject It was happend when autoNotify is triggered by setAttribute Change-Id: Ib4901839c524ff54d0ff2366914a5f9201849016 Signed-off-by: coderhyme Reviewed-on: https://gerrit.iotivity.org/gerrit/2061 Tested-by: jenkins-iotivity Reviewed-by: Madan Lanka --- .../include/RCSResourceObject.h | 48 +++++---- .../src/serverBuilder/src/RCSResourceObject.cpp | 117 +++++++++++---------- 2 files changed, 86 insertions(+), 79 deletions(-) diff --git a/service/resource-encapsulation/include/RCSResourceObject.h b/service/resource-encapsulation/include/RCSResourceObject.h index 9a46525..7c0c636 100755 --- a/service/resource-encapsulation/include/RCSResourceObject.h +++ b/service/resource-encapsulation/include/RCSResourceObject.h @@ -198,20 +198,20 @@ namespace OIC class LockGuard; - typedef std::function < RCSGetResponse(const RCSRequest &, - RCSResourceAttributes &) > GetRequestHandler; - typedef std::function < RCSSetResponse(const RCSRequest &, - RCSResourceAttributes &) > SetRequestHandler; + typedef std::function < RCSGetResponse(const RCSRequest&, + RCSResourceAttributes&) > GetRequestHandler; + typedef std::function < RCSSetResponse(const RCSRequest&, + RCSResourceAttributes&) > SetRequestHandler; - typedef std::function < void(const RCSResourceAttributes::Value &, + typedef std::function < void(const RCSResourceAttributes::Value&, const RCSResourceAttributes::Value &) > AttributeUpdatedListener; public: RCSResourceObject(RCSResourceObject&&) = delete; - RCSResourceObject(const RCSResourceObject &) = delete; + RCSResourceObject(const RCSResourceObject&) = delete; - RCSResourceObject &operator=(RCSResourceObject && ) = delete; - RCSResourceObject &operator=(const RCSResourceObject &) = delete; + RCSResourceObject& operator=(RCSResourceObject&&) = delete; + RCSResourceObject& operator=(const RCSResourceObject&) = delete; virtual ~RCSResourceObject(); @@ -223,17 +223,17 @@ namespace OIC * * @note It is guaranteed thread-safety about attributes. */ - void setAttribute(const std::string &key, const RCSResourceAttributes::Value & value); + void setAttribute(const std::string& key, const RCSResourceAttributes::Value& value); /** * @overload */ - void setAttribute(const std::string &key, RCSResourceAttributes::Value&& value); + void setAttribute(const std::string& key, RCSResourceAttributes::Value&& value); /** * @overload */ - void setAttribute(std::string&& key, const RCSResourceAttributes::Value & value); + void setAttribute(std::string&& key, const RCSResourceAttributes::Value& value); /** * @overload @@ -252,7 +252,7 @@ namespace OIC * @throw InvalidKeyException * Throw exception when empty string is provided as Attribute key. */ - RCSResourceAttributes::Value getAttributeValue(const std::string &key) const; + RCSResourceAttributes::Value getAttributeValue(const std::string& key) const; /** * API for retrieving the attribute value associated with the supplied name. @@ -264,7 +264,7 @@ namespace OIC * It is guaranteed thread-safety about attributes. */ template< typename T > - T getAttribute(const std::string &key) const + T getAttribute(const std::string& key) const { WeakGuard lock(*this); return m_resourceAttributes.at(key).get< T >(); @@ -279,7 +279,7 @@ namespace OIC * * It is guaranteed thread-safety about attributes. */ - bool removeAttribute(const std::string &key); + bool removeAttribute(const std::string& key); /** * API for checking whether a particular attribute is there for a resource or not. @@ -290,7 +290,7 @@ namespace OIC * * It is guaranteed thread-safety about attributes. */ - bool containsAttribute(const std::string &key) const; + bool containsAttribute(const std::string& key) const; /** * API for getting all the attributes of the RCSResourceObject. @@ -305,12 +305,12 @@ namespace OIC * @throw NoLockException * If you don't do lock with LockGuard, throw exception. */ - RCSResourceAttributes &getAttributes(); + RCSResourceAttributes& getAttributes(); /** * @overload */ - const RCSResourceAttributes &getAttributes() const; + const RCSResourceAttributes& getAttributes() const; /** * API for checking whether the particular resource is observable or not @@ -354,7 +354,7 @@ namespace OIC * @param listener Listener for updation of the interested attribute * */ - virtual void addAttributeUpdatedListener(const std::string &key, + virtual void addAttributeUpdatedListener(const std::string& key, AttributeUpdatedListener listener); /** @@ -364,7 +364,7 @@ namespace OIC * @param listener Listener for updation of the interested attribute * */ - virtual void addAttributeUpdatedListener(std::string &&key, + virtual void addAttributeUpdatedListener(std::string&& key, AttributeUpdatedListener listener); /** @@ -373,7 +373,7 @@ namespace OIC * @param key The interested attribute's key * */ - virtual bool removeAttributeUpdatedListener(const std::string &key); + virtual bool removeAttributeUpdatedListener(const std::string& key); /** * API for notifying all observers of the RCSResourceObject @@ -432,8 +432,14 @@ namespace OIC OCEntityHandlerResult handleObserve(std::shared_ptr< OC::OCResourceRequest >); void expectOwnLock() const; + void autoNotify(bool, AutoNotifyPolicy) const; - void autoNotifyIfNeeded(const std::string& , const RCSResourceAttributes::Value& ); + void autoNotify(bool) const; + + bool testValueUpdated(const std::string&, const RCSResourceAttributes::Value&) const; + + template< typename K, typename V > + void setAttributeInternal(K&&, V&&); private: const uint8_t m_properties; diff --git a/service/resource-encapsulation/src/serverBuilder/src/RCSResourceObject.cpp b/service/resource-encapsulation/src/serverBuilder/src/RCSResourceObject.cpp index a41f41f..1d3a0cb 100755 --- a/service/resource-encapsulation/src/serverBuilder/src/RCSResourceObject.cpp +++ b/service/resource-encapsulation/src/serverBuilder/src/RCSResourceObject.cpp @@ -174,18 +174,11 @@ namespace OIC OC::EntityHandler entityHandler{ std::bind(&RCSResourceObject::entityHandler, server.get(), std::placeholders::_1) }; - try - { - typedef OCStackResult (*RegisterResource)(OCResourceHandle&, std::string&, - const std::string&, const std::string&, OC::EntityHandler, uint8_t); + typedef OCStackResult (*RegisterResource)(OCResourceHandle&, std::string&, + const std::string&, const std::string&, OC::EntityHandler, uint8_t); - invokeOCFunc(static_cast(OC::OCPlatform::registerResource), - handle, m_uri, m_type, m_interface, entityHandler, m_properties); - } - catch (OC::OCException& e) - { - throw PlatformException(e.code()); - } + invokeOCFunc(static_cast(OC::OCPlatform::registerResource), + handle, m_uri, m_type, m_interface, entityHandler, m_properties); server->m_resourceHandle = handle; @@ -223,56 +216,48 @@ namespace OIC } } - void RCSResourceObject::setAttribute(const std::string& key, - const RCSResourceAttributes::Value& value) + template< typename K, typename V > + void RCSResourceObject::setAttributeInternal(K&& key, V&& value) { - WeakGuard lock(*this); + bool needToNotify = false; + bool valueUpdated = false; - if(lock.hasLocked()) { - autoNotifyIfNeeded(key, value); + WeakGuard lock(*this); + + if (lock.hasLocked()) + { + needToNotify = true; + valueUpdated = testValueUpdated(key, value); + } + + m_resourceAttributes[std::forward< K >(key)] = std::forward< V >(value); } - m_resourceAttributes[key] = value; + if (needToNotify) autoNotify(valueUpdated); + } + void RCSResourceObject::setAttribute(const std::string& key, + const RCSResourceAttributes::Value& value) + { + setAttributeInternal(key, value); } void RCSResourceObject::setAttribute(const std::string& key, RCSResourceAttributes::Value&& value) { - WeakGuard lock(*this); - - if(lock.hasLocked()) - { - autoNotifyIfNeeded(key, value); - } - - m_resourceAttributes[key] = std::move(value); + setAttributeInternal(key, std::move(value)); } void RCSResourceObject::setAttribute(std::string&& key, const RCSResourceAttributes::Value& value) { - WeakGuard lock(*this); - - if(lock.hasLocked()) - { - autoNotifyIfNeeded(key, value); - } - - m_resourceAttributes[std::move(key)] = value; + setAttributeInternal(std::move(key), value); } void RCSResourceObject::setAttribute(std::string&& key, RCSResourceAttributes::Value&& value) { - WeakGuard lock(*this); - - if(lock.hasLocked()) - { - autoNotifyIfNeeded(key, value); - } - - m_resourceAttributes[std::move(key)] = std::move(value); + setAttributeInternal(std::move(key), std::move(value)); } RCSResourceAttributes::Value RCSResourceObject::getAttributeValue( @@ -284,13 +269,21 @@ namespace OIC bool RCSResourceObject::removeAttribute(const std::string& key) { - WeakGuard lock(*this); - if (m_resourceAttributes.erase(key)) + bool needToNotify = false; + bool erased = false; { - autoNotify(true, getAutoNotifyPolicy()); - return true; + WeakGuard lock(*this); + + if (m_resourceAttributes.erase(key)) + { + erased = true; + needToNotify = lock.hasLocked(); + } } - return false; + + if (needToNotify) autoNotify(true); + + return erased; } bool RCSResourceObject::containsAttribute(const std::string& key) const @@ -343,8 +336,7 @@ namespace OIC { typedef OCStackResult (*NotifyAllObservers)(OCResourceHandle); - invokeOCFuncWithResultExpect( - { OC_STACK_OK, OC_STACK_NO_OBSERVERS }, + invokeOCFuncWithResultExpect({ OC_STACK_OK, OC_STACK_NO_OBSERVERS }, static_cast< NotifyAllObservers >(OC::OCPlatform::notifyAllObservers), m_resourceHandle); } @@ -366,15 +358,15 @@ namespace OIC bool RCSResourceObject::removeAttributeUpdatedListener(const std::string& key) { std::lock_guard lock(m_mutexKeyAttributeUpdate); - return (bool) m_keyAttributesUpdatedListeners.erase(key); + + return m_keyAttributesUpdatedListeners.erase(key) != 0; } - void RCSResourceObject::autoNotifyIfNeeded(const std::string& key, - const RCSResourceAttributes::Value& value) + bool RCSResourceObject::testValueUpdated(const std::string& key, + const RCSResourceAttributes::Value& value) const { - autoNotify( m_resourceAttributes.contains(key) == false - || m_resourceAttributes.at(key) != value - , m_autoNotifyPolicy); + return m_resourceAttributes.contains(key) == false + || m_resourceAttributes.at(key) != value; } void RCSResourceObject::setAutoNotifyPolicy(AutoNotifyPolicy policy) @@ -397,11 +389,18 @@ namespace OIC return m_setRequestHandlerPolicy; } + void RCSResourceObject::autoNotify(bool isAttributesChanged) const + { + autoNotify(isAttributesChanged, m_autoNotifyPolicy); + } + void RCSResourceObject::autoNotify( bool isAttributesChanged, AutoNotifyPolicy autoNotifyPolicy) const { if(autoNotifyPolicy == AutoNotifyPolicy::NEVER) return; - if(autoNotifyPolicy == AutoNotifyPolicy::UPDATED && isAttributesChanged == false) return; + if(autoNotifyPolicy == AutoNotifyPolicy::UPDATED && + isAttributesChanged == false) return; + notify(); } @@ -537,9 +536,9 @@ namespace OIC RCSResourceObject::LockGuard::LockGuard( const RCSResourceObject& resourceObject, AutoNotifyPolicy autoNotifyPolicy) : - m_resourceObject(resourceObject), - m_autoNotifyPolicy { autoNotifyPolicy }, - m_isOwningLock{ false } + m_resourceObject(resourceObject), + m_autoNotifyPolicy { autoNotifyPolicy }, + m_isOwningLock{ false } { init(); } @@ -575,6 +574,7 @@ namespace OIC if (resourceObject.m_lockOwner != std::this_thread::get_id()) { m_resourceObject.m_mutex.lock(); + m_resourceObject.m_lockOwner = std::this_thread::get_id(); m_isOwningLock = true; } } @@ -583,6 +583,7 @@ namespace OIC { if (m_isOwningLock) { + m_resourceObject.m_lockOwner = std::thread::id{ }; m_resourceObject.m_mutex.unlock(); } } -- 2.7.4