From 4b07f97ca8eea4d71c010afead955b2b7f16b178 Mon Sep 17 00:00:00 2001 From: Walter Gray Date: Fri, 15 May 2015 12:51:31 -0700 Subject: [PATCH 1/4] Added a unit test to demonstrate a potential double-lock behavior --- src/autowiring/test/ContextCreatorTest.cpp | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/autowiring/test/ContextCreatorTest.cpp b/src/autowiring/test/ContextCreatorTest.cpp index 7420a172c..4748d6ef5 100644 --- a/src/autowiring/test/ContextCreatorTest.cpp +++ b/src/autowiring/test/ContextCreatorTest.cpp @@ -200,3 +200,35 @@ TEST_F(ContextCreatorTest, VoidKeyType) { EXPECT_EQ(0UL, vc->GetSize()) << "A void context creator was not correctly updated when its dependent context went out of scope"; EXPECT_EQ(2UL, vc->m_totalDestroyed) << "The void creator did not receive the expected number of NotifyContextDestroyed calls"; } + +struct mySigil {}; + +class BackReferencingClass: + public CoreRunnable +{ +public: + bool OnStart(void) override { + AutoCurrentContext ctxt; + m_parentContext = ctxt->GetParentContext(); + return true; + } + void OnStop(bool graceful) { + m_parentContext.reset(); + } + std::shared_ptr m_parentContext; +}; + +TEST_F(ContextCreatorTest, TeardownListenerTest) { + // Create a context and verify teardown happens as expected + AutoCreateContext mainContext; + CurrentContextPusher pusher(mainContext); + + AutoRequired> creator; + { + auto subctxt = creator->CreateContext(0).first; + auto brc = subctxt->Inject(); + subctxt->Initiate(); + } + creator->Clear(true); + ASSERT_TRUE(true) << "Really all this test has to do is not crash by this point."; +} \ No newline at end of file From 665345f754255088975c1caeb9ba110ba4f5a4f8 Mon Sep 17 00:00:00 2001 From: Walter Gray Date: Fri, 15 May 2015 13:25:40 -0700 Subject: [PATCH 2/4] Fixed the problem using a local copy, cut down on code duplication --- autowiring/ContextCreatorBase.h | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/autowiring/ContextCreatorBase.h b/autowiring/ContextCreatorBase.h index 93cae6d10..29970b24a 100644 --- a/autowiring/ContextCreatorBase.h +++ b/autowiring/ContextCreatorBase.h @@ -28,31 +28,17 @@ class ContextCreatorBase { /// template void Clear(bool wait, Ctr& ctr, Fx&& locker) { - if(!wait) { - // Trivial signal-clear-return: - std::lock_guard lk(m_contextLock); - for(const auto& ctxt : ctr) { - auto locked = locker(ctxt); - if(locked) - locked->SignalShutdown(); - } - ctr.clear(); - return; + // Copy out under lock + Ctr ctrCopy = (std::lock_guard(m_contextLock), std::move(ctr)); + + for (const auto& ctxt : ctrCopy) { + auto locked = locker(ctxt); + if(locked) + locked->SignalShutdown(); } - Ctr ctrCopy; - - // Copy out and clear: - { - std::lock_guard lk(m_contextLock); - for(const auto& ctxt : ctr) { - auto locked = locker(ctxt); - if(locked) - locked->SignalShutdown(); - } - ctrCopy = ctr; - ctr.clear(); - } + if (!wait) + return; // Signal everyone first, then wait in a second pass: for(const auto& ctxt : ctrCopy) { From fbc43b8ced5a7d275af4631910c6db757335c9f2 Mon Sep 17 00:00:00 2001 From: Walter Gray Date: Fri, 15 May 2015 14:00:45 -0700 Subject: [PATCH 3/4] minor commenting and naming fixes --- autowiring/ContextCreatorBase.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/autowiring/ContextCreatorBase.h b/autowiring/ContextCreatorBase.h index 29970b24a..5e4768b4c 100644 --- a/autowiring/ContextCreatorBase.h +++ b/autowiring/ContextCreatorBase.h @@ -24,14 +24,16 @@ class ContextCreatorBase { /// The collection of contexts to be cleared /// An iterator locker, which can obtain a shared pointer from an iterator /// - /// This method is synchronized on the contextLock and this will generally make it thread-safe + /// This method is synchronized on the contextLock and this will generally make it thread-safe. + /// It will clear the local context list immediately, but it is not garunteed that the context + /// list will be empty at the time of return. /// template void Clear(bool wait, Ctr& ctr, Fx&& locker) { - // Copy out under lock - Ctr ctrCopy = (std::lock_guard(m_contextLock), std::move(ctr)); + // Move out under lock + Ctr ctrLocal = (std::lock_guard(m_contextLock), std::move(ctr)); - for (const auto& ctxt : ctrCopy) { + for (const auto& ctxt : ctrLocal) { auto locked = locker(ctxt); if(locked) locked->SignalShutdown(); @@ -41,7 +43,7 @@ class ContextCreatorBase { return; // Signal everyone first, then wait in a second pass: - for(const auto& ctxt : ctrCopy) { + for (const auto& ctxt : ctrLocal) { auto locked = locker(ctxt); if(locked) locked->Wait(); From b00e1a33b410ddd8b5763e3cdbeb714419967497 Mon Sep 17 00:00:00 2001 From: Walter Gray Date: Fri, 15 May 2015 14:10:10 -0700 Subject: [PATCH 4/4] Simplified the test --- src/autowiring/test/ContextCreatorTest.cpp | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/autowiring/test/ContextCreatorTest.cpp b/src/autowiring/test/ContextCreatorTest.cpp index 4748d6ef5..34943ed44 100644 --- a/src/autowiring/test/ContextCreatorTest.cpp +++ b/src/autowiring/test/ContextCreatorTest.cpp @@ -203,19 +203,11 @@ TEST_F(ContextCreatorTest, VoidKeyType) { struct mySigil {}; -class BackReferencingClass: +class Runnable: public CoreRunnable { public: - bool OnStart(void) override { - AutoCurrentContext ctxt; - m_parentContext = ctxt->GetParentContext(); - return true; - } - void OnStop(bool graceful) { - m_parentContext.reset(); - } - std::shared_ptr m_parentContext; + bool OnStart(void) override { return true; } }; TEST_F(ContextCreatorTest, TeardownListenerTest) { @@ -226,7 +218,7 @@ TEST_F(ContextCreatorTest, TeardownListenerTest) { AutoRequired> creator; { auto subctxt = creator->CreateContext(0).first; - auto brc = subctxt->Inject(); + auto brc = subctxt->Inject(); subctxt->Initiate(); } creator->Clear(true);