From 9620b89aec2f9fdce92a593c556693a5802708ea Mon Sep 17 00:00:00 2001 From: coderhyme Date: Wed, 15 Jul 2015 00:01:46 +0900 Subject: [PATCH] Remove DeadLockException from ResourceObject It allows recursive lock from now, instead. Change-Id: I349c9dd9f2916d73587f814ac98d088790b4e09e Signed-off-by: coderhyme Reviewed-on: https://gerrit.iotivity.org/gerrit/1647 Tested-by: jenkins-iotivity Reviewed-by: Uze Choi --- .../src/serverBuilder/include/ResourceObject.h | 14 +- .../src/serverBuilder/src/ResourceObject.cpp | 50 +++--- .../serverBuilder/unittests/ResourceObjectTest.cpp | 172 ++++++++++++++++----- 3 files changed, 169 insertions(+), 67 deletions(-) diff --git a/service/resource-manipulation/src/serverBuilder/include/ResourceObject.h b/service/resource-manipulation/src/serverBuilder/include/ResourceObject.h index 8da8e23..979ce4a 100755 --- 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 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 --- 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 --- 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(OCPlatform::registerResource)) + mocks.OnCallFuncOverload(static_cast< registerResource >(OCPlatform::registerResource)) .Return(OC_STACK_OK); } }; TEST_F(ResourceObjectBuilderTest, RegisterResourceWhenCallCreate) { - mocks.ExpectCallFuncOverload(static_cast(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(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(OCPlatform::registerResource)).Do( + static_cast(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); public: -- 2.7.4