Fix deadlock issue from RCSResourceObject

It was happend when autoNotify is triggered by setAttribute

Change-Id: Ib4901839c524ff54d0ff2366914a5f9201849016
Signed-off-by: coderhyme <jhyo.kim@samsung.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/2061
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Madan Lanka <lanka.madan@samsung.com>

diff --git a/service/resource-encapsulation/include/RCSResourceObject.h b/service/resource-encapsulation/include/RCSResourceObject.h
index 9a46525..7c0c636 100755 (executable)
--- 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 (executable)
--- 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<RegisterResource>(OC::OCPlatform::registerResource),
-                        handle, m_uri, m_type, m_interface, entityHandler, m_properties);
-            }
-            catch (OC::OCException& e)
-            {
-                throw PlatformException(e.code());
-            }
+            invokeOCFunc(static_cast<RegisterResource>(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<std::mutex> 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();
             }
         }