Remove DeadLockException from ResourceObject

It allows recursive lock from now, instead.

Change-Id: I349c9dd9f2916d73587f814ac98d088790b4e09e
Signed-off-by: coderhyme <jhyo.kim@samsung.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/1647
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Uze Choi <uzchoi@samsung.com>

diff --git a/service/resource-manipulation/src/serverBuilder/include/ResourceObject.h b/service/resource-manipulation/src/serverBuilder/include/ResourceObject.h
index 8da8e23..979ce4a 100755 (executable)
--- a/service/resource-manipulation/src/serverBuilder/include/ResourceObject.h
+++ b/service/resource-manipulation/src/serverBuilder/include/ResourceObject.h
@@ -46,12 +46,6 @@ namespace OIC
             NoLockException(std::string&& what) : PrimitiveException{ std::move(what) } {}
         };
 
-        class DeadLockException: public PrimitiveException
-        {
-        public:
-            DeadLockException(std::string&& what) : PrimitiveException{ std::move(what) } {}
-        };
-
         class ResourceObject
         {
         private:
@@ -211,7 +205,11 @@ namespace OIC
 
         private:
             const ResourceObject& m_resourceObject;
+
             AutoNotifyPolicy m_autoNotifyPolicy;
+
+            bool m_isOwningLock;
+
             std::function<void()> m_autoNotifyFunc;
         };
 
@@ -230,8 +228,8 @@ namespace OIC
             bool hasLocked() const;
 
         private:
-            bool m_hasLocked;
-            const ResourceObject& m_serverResource;
+            bool m_isOwningLock;
+            const ResourceObject& m_resourceObject;
         };
     }
 }

diff --git a/service/resource-manipulation/src/serverBuilder/src/ResourceObject.cpp b/service/resource-manipulation/src/serverBuilder/src/ResourceObject.cpp
index e23902e..389d108 100755 (executable)
--- a/service/resource-manipulation/src/serverBuilder/src/ResourceObject.cpp
+++ b/service/resource-manipulation/src/serverBuilder/src/ResourceObject.cpp
@@ -197,7 +197,7 @@ namespace OIC
                 m_resourceAttributes{ std::move(attrs) },
                 m_getRequestHandler{ },
                 m_setRequestHandler{ },
-                m_autoNotifyPolicy { AutoNotifyPolicy::ALWAYS },
+                m_autoNotifyPolicy { AutoNotifyPolicy::UPDATED },
                 m_setRequestHandlerPolicy { SetRequestHandlerPolicy::DEFAULT },
                 m_keyAttributesUpdatedHandlers{ },
                 m_lockOwner{ },
@@ -438,7 +438,7 @@ namespace OIC
                 return handleRequestGet(request);
             }
 
-            if (request->getRequestType() == "PUT" || request->getRequestType() == "POST")
+            if (request->getRequestType() == "PUT")
             {
                 return handleRequestSet(request);
             }
@@ -505,7 +505,7 @@ namespace OIC
 
         ResourceObject::LockGuard::LockGuard(
                 const ResourceObject& serverResource) :
-                LockGuard{ serverResource, serverResource.getAutoNotifyPolicy()}
+                LockGuard{ serverResource, serverResource.getAutoNotifyPolicy() }
         {
         }
 
@@ -516,51 +516,55 @@ namespace OIC
         }
 
         ResourceObject::LockGuard::LockGuard(
-                const ResourceObject& serverResource, AutoNotifyPolicy autoNotifyPolicy) :
-                        m_resourceObject(serverResource),
-                        m_autoNotifyPolicy(autoNotifyPolicy)
+                const ResourceObject& resourceObject, AutoNotifyPolicy autoNotifyPolicy) :
+                        m_resourceObject(resourceObject),
+                        m_autoNotifyPolicy { autoNotifyPolicy },
+                        m_isOwningLock{ false }
         {
-            if (m_resourceObject.m_lockOwner == std::this_thread::get_id())
+            if (resourceObject.m_lockOwner != std::this_thread::get_id())
             {
-                throw DeadLockException{ "Can't lock recursively in same thread." };
+                m_resourceObject.m_mutex.lock();
+                m_resourceObject.m_lockOwner = std::this_thread::get_id();
+                m_isOwningLock = true;
             }
-
-            m_resourceObject.m_mutex.lock();
-            m_resourceObject.m_lockOwner = std::this_thread::get_id();
-
             m_autoNotifyFunc = ::createAutoNotifyInvoker(&ResourceObject::autoNotify,
                     m_resourceObject, m_resourceObject.m_resourceAttributes, m_autoNotifyPolicy);
         }
 
         ResourceObject::LockGuard::~LockGuard()
         {
-            if(m_autoNotifyFunc)    m_autoNotifyFunc();
-            m_resourceObject.m_lockOwner = std::thread::id();
-            m_resourceObject.m_mutex.unlock();
+            if (m_autoNotifyFunc) m_autoNotifyFunc();
+
+            if (m_isOwningLock)
+            {
+                m_resourceObject.m_lockOwner = std::thread::id{ };
+                m_resourceObject.m_mutex.unlock();
+            }
         }
 
         ResourceObject::WeakGuard::WeakGuard(
-                const ResourceObject& serverResource) :
-                m_hasLocked{ false }, m_serverResource(serverResource)
+                const ResourceObject& resourceObject) :
+                m_isOwningLock{ false },
+                m_resourceObject(resourceObject)
         {
-            if (m_serverResource.m_lockOwner != std::this_thread::get_id())
+            if (resourceObject.m_lockOwner != std::this_thread::get_id())
             {
-                m_serverResource.m_mutex.lock();
-                m_hasLocked = true;
+                m_resourceObject.m_mutex.lock();
+                m_isOwningLock = true;
             }
         }
 
         ResourceObject::WeakGuard::~WeakGuard()
         {
-            if (m_hasLocked)
+            if (m_isOwningLock)
             {
-                m_serverResource.m_mutex.unlock();
+                m_resourceObject.m_mutex.unlock();
             }
         }
 
         bool ResourceObject::WeakGuard::hasLocked() const
         {
-            return m_hasLocked;
+            return m_isOwningLock;
         }
     }
 }

diff --git a/service/resource-manipulation/src/serverBuilder/unittests/ResourceObjectTest.cpp b/service/resource-manipulation/src/serverBuilder/unittests/ResourceObjectTest.cpp
index e994b5f..b742583 100755 (executable)
--- a/service/resource-manipulation/src/serverBuilder/unittests/ResourceObjectTest.cpp
+++ b/service/resource-manipulation/src/serverBuilder/unittests/ResourceObjectTest.cpp
@@ -33,16 +33,19 @@ using namespace testing;
 using namespace OIC::Service;
 using namespace OC;
 
-typedef OCStackResult (*registerResourceSig)(OCResourceHandle&,
+using registerResource = OCStackResult (*)(OCResourceHandle&,
                        string&,
                        const string&,
                        const string&,
                        EntityHandler,
                        uint8_t );
 
-static constexpr char RESOURCE_URI[]{ "a/test" };
-static constexpr char RESOURCE_TYPE[]{ "resourceType" };
-static constexpr char KEY[]{ "key" };
+using NotifyAllObservers = OCStackResult (*)(OCResourceHandle);
+
+constexpr char RESOURCE_URI[]{ "a/test" };
+constexpr char RESOURCE_TYPE[]{ "resourceType" };
+constexpr char KEY[]{ "key" };
+constexpr int value{ 100 };
 
 TEST(ResourceObjectBuilderCreateTest, ThrowIfUriIsInvalid)
 {
@@ -57,15 +60,16 @@ public:
 protected:
     void SetUp() override
     {
-        mocks.OnCallFuncOverload(static_cast<registerResourceSig>(OCPlatform::registerResource))
+        mocks.OnCallFuncOverload(static_cast< registerResource >(OCPlatform::registerResource))
                 .Return(OC_STACK_OK);
     }
 };
 
 TEST_F(ResourceObjectBuilderTest, RegisterResourceWhenCallCreate)
 {
-    mocks.ExpectCallFuncOverload(static_cast<registerResourceSig>(OCPlatform::registerResource))
+    mocks.ExpectCallFuncOverload(static_cast< registerResource >(OCPlatform::registerResource))
             .Return(OC_STACK_OK);
+
     ResourceObject::Builder(RESOURCE_URI, RESOURCE_TYPE, "").build();
 }
 
@@ -86,7 +90,7 @@ TEST_F(ResourceObjectBuilderTest, ResourceServerHasAttrsSetByBuilder)
     auto serverResource = ResourceObject::Builder(RESOURCE_URI, RESOURCE_TYPE, "").
             setAttributes(attrs).build();
 
-    ResourceObject::LockGuard lock{ serverResource };
+    ResourceObject::LockGuard lock{ serverResource, ResourceObject::AutoNotifyPolicy::NEVER };
     EXPECT_EQ(attrs, serverResource->getAttributes());
 }
 
@@ -101,22 +105,27 @@ protected:
     void SetUp() override
     {
         initMocks();
+
         server = ResourceObject::Builder(RESOURCE_URI, RESOURCE_TYPE, "").build();
+
+        initResourceObject();
     }
 
     virtual void initMocks()
     {
-        mocks.OnCallFuncOverload(static_cast< registerResourceSig >(OCPlatform::registerResource)).
+        mocks.OnCallFuncOverload(static_cast< registerResource >(OCPlatform::registerResource)).
                 Return(OC_STACK_OK);
 
         mocks.OnCallFunc(OCPlatform::unregisterResource).Return(OC_STACK_OK);
     }
+
+    virtual void initResourceObject() {
+        server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::NEVER);
+    }
 };
 
 TEST_F(ResourceObjectTest, AccessAttributesWithLock)
 {
-    constexpr int value{ 100 };
-
     {
         ResourceObject::LockGuard lock{ server };
         auto& attr = server->getAttributes();
@@ -126,24 +135,15 @@ TEST_F(ResourceObjectTest, AccessAttributesWithLock)
     ASSERT_EQ(value, server->getAttribute<int>(KEY));
 }
 
-TEST_F(ResourceObjectTest, ThrowIfTryToAccessAttributesWithoutLock)
+TEST_F(ResourceObjectTest, ThrowIfTryToAccessAttributesWithoutGuard)
 {
     ASSERT_THROW(server->getAttributes(), NoLockException);
 }
 
-TEST_F(ResourceObjectTest, ThrowIfLockRecursively)
-{
-    ResourceObject::LockGuard lock{ server };
-
-    ASSERT_THROW(ResourceObject::LockGuard again{ server }, DeadLockException);
-}
-
-TEST_F(ResourceObjectTest, AccessingAttributesWithMethodsWithinLockDoesntCauseDeadLock)
+TEST_F(ResourceObjectTest, SettingAttributesWithinGuardDoesntCauseDeadLock)
 {
-    constexpr int value{ 100 };
-
     {
-        ResourceObject::LockGuard lock{ server };
+        ResourceObject::LockGuard guard{ server };
         server->setAttribute(KEY, value);
     }
 
@@ -153,32 +153,87 @@ TEST_F(ResourceObjectTest, AccessingAttributesWithMethodsWithinLockDoesntCauseDe
 
 class AutoNotifyTest: public ResourceObjectTest
 {
-
-public:
-    using NotifyAllObservers = OCStackResult (*)(OCResourceHandle);
-    int value{ 100 };
-
 protected:
     void initMocks() override
     {
-        mocks.OnCallFuncOverload(
-                static_cast< NotifyAllObservers >
-                            (OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+        mocks.OnCallFuncOverload(static_cast< NotifyAllObservers >(
+                OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+    }
+
+    virtual void initResourceObject() {
+        // intended blank
     }
 };
 
-TEST_F(AutoNotifyTest, DefalutAutoNotifyPolicyIsAlways)
+TEST_F(AutoNotifyTest, DefalutAutoNotifyPolicyIsUpdated)
 {
-    ASSERT_EQ(ResourceObject::AutoNotifyPolicy::ALWAYS, server->getAutoNotifyPolicy());
+    ASSERT_EQ(ResourceObject::AutoNotifyPolicy::UPDATED, server->getAutoNotifyPolicy());
 }
 
-TEST_F(AutoNotifyTest, SetAutoNotifyPolicyBySetter)
+TEST_F(AutoNotifyTest, AutoNotifyPolicyCanBeSet)
 {
     server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::NEVER);
 
-    ASSERT_EQ(ResourceObject::AutoNotifyPolicy::NEVER,server->getAutoNotifyPolicy());
+    ASSERT_EQ(ResourceObject::AutoNotifyPolicy::NEVER, server->getAutoNotifyPolicy());
 }
 
+TEST_F(AutoNotifyTest, WithUpdatedPolicy_NeverBeNotifiedIfAttributeIsNotChanged)
+{
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::UPDATED);
+    server->setAttribute(KEY, value);
+
+    mocks.NeverCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers));
+
+    server->setAttribute(KEY, value);
+}
+
+TEST_F(AutoNotifyTest, WithUpdatedPolicy_WillBeNotifiedIfAttributeIsChanged)
+{
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::UPDATED);
+    server->setAttribute(KEY, value);
+
+    mocks.ExpectCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+
+    server->setAttribute(KEY, value + 1);
+}
+
+TEST_F(AutoNotifyTest, WithUpdatedPolicy_WillBeNotifiedIfValueIsAdded)
+{
+    constexpr char newKey[]{ "newKey" };
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::UPDATED);
+
+    mocks.ExpectCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+
+    server->setAttribute(newKey, value);
+}
+
+TEST_F(AutoNotifyTest, WithNeverPolicy_NeverBeNotifiedEvenIfAttributeIsChanged)
+{
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::NEVER);
+
+    mocks.NeverCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers));
+
+    ResourceObject::LockGuard lock{ server };
+    server->setAttribute(KEY, value);
+}
+
+TEST_F(AutoNotifyTest, WithAlwaysPolicy_WillBeNotifiedEvenIfAttributeIsNotChanged)
+{
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::ALWAYS);
+    server->setAttribute(KEY, value);
+
+    mocks.ExpectCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+
+    server->setAttribute(KEY, value);
+}
+
+
+
 TEST_F(AutoNotifyTest, WorkingWithNeverPolicyWhenAttributesNoChangeByGetAttributes)
 {
     server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::NEVER);
@@ -332,6 +387,52 @@ TEST_F(AutoNotifyTest, WorkingWithUpdatedPolicyWhenAttributesChangeBySetAttribut
 }
 
 
+class AutoNotifyWithGuardTest: public AutoNotifyTest
+{
+};
+
+TEST_F(AutoNotifyWithGuardTest, GuardFollowsServerPolicyByDefault)
+{
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::UPDATED);
+
+    mocks.ExpectCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+
+    ResourceObject::LockGuard guard{ server };
+    server->setAttribute(KEY, value);
+}
+
+TEST_F(AutoNotifyWithGuardTest, GuardCanOverridePolicy)
+{
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::ALWAYS);
+
+    mocks.NeverCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers));
+
+    ResourceObject::LockGuard guard{ server, ResourceObject::AutoNotifyPolicy::NEVER };
+    server->getAttributes()[KEY] = value;
+}
+
+TEST_F(AutoNotifyWithGuardTest, GuardInvokesNotifyWhenDestroyed)
+{
+    server->setAutoNotifyPolicy(ResourceObject::AutoNotifyPolicy::NEVER);
+
+    mocks.ExpectCallFuncOverload(static_cast< NotifyAllObservers >(
+            OC::OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+
+    {
+        ResourceObject::LockGuard guard{ server, ResourceObject::AutoNotifyPolicy::ALWAYS };
+        server->setAttribute(KEY, value);
+    }
+
+    mocks.NeverCallFuncOverload(static_cast< NotifyAllObservers >(
+               OC::OCPlatform::notifyAllObservers)).Return(OC_STACK_OK);
+
+    server->setAttribute(KEY, value);
+}
+
+
+
 class ResourceObjectHandlingRequestTest: public ResourceObjectTest
 {
 public:
@@ -374,7 +475,7 @@ protected:
     void initMocks() override
     {
         mocks.OnCallFuncOverload(
-            static_cast<registerResourceSig>(OCPlatform::registerResource)).Do(
+            static_cast<registerResource>(OCPlatform::registerResource)).Do(
                     bind(&ResourceObjectHandlingRequestTest::registerResourceFake,
                             this, _1, _2, _3, _4, _5, _6));
         mocks.OnCallFunc(OCPlatform::unregisterResource).Return(OC_STACK_OK);
@@ -462,7 +563,6 @@ TEST_F(ResourceObjectHandlingRequestTest, SendSetResponseWithCustomAttrsAndResul
 class AutoNotifySetHandlingRequestTest: public ResourceObjectHandlingRequestTest
 {
 public:
-    using NotifyAllObservers = OCStackResult (*)(OCResourceHandle);
     using SendResponse = OCStackResult (*)(std::shared_ptr<OCResourceResponse>);
 
 public: