From 97f0e622d88af02c71975e81124ef930f0b9837b Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Thu, 21 Aug 2014 16:57:49 -0700 Subject: [PATCH 1/4] Adding unit test demonstrating failure of PostConstruction NotifyWhenAutowired for Context. --- src/autowiring/test/PostConstructTest.cpp | 25 ++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/autowiring/test/PostConstructTest.cpp b/src/autowiring/test/PostConstructTest.cpp index d20a9692d..5c948a38a 100644 --- a/src/autowiring/test/PostConstructTest.cpp +++ b/src/autowiring/test/PostConstructTest.cpp @@ -264,10 +264,9 @@ TEST_F(PostConstructTest, ContextNotifyWhenAutowired) { // Now we'd like to be notified when SimpleObject gets added: ctxt->NotifyWhenAutowired( - [called] { + [called] { *called = true; - } - ); + }); // Should only be two uses, at this point, of the capture of the above lambda: EXPECT_EQ(2L, called.use_count()) << "Unexpected number of references held in a capture lambda"; @@ -283,3 +282,23 @@ TEST_F(PostConstructTest, ContextNotifyWhenAutowired) { ASSERT_TRUE(called.unique()) << "Autowiring notification lambda was not properly cleaned up"; } +TEST_F(PostConstructTest, ContextNotifyWhenAutowiredPostConstruct) { + auto called = std::make_shared(false); + AutoCurrentContext ctxt; + + // Create an object that will satisfy subsequent notification call: + AutoRequired sobj; + + // Notification should be immediate: + ctxt->NotifyWhenAutowired( + [called] { + *called = true; + }); + + // Insert the SimpleObject, see if the lambda got hit: + ASSERT_TRUE(*called) << "Context-wide autowiring notification was not hit as expected when a matching type was injected into a context"; + + // Our shared pointer should be unique by this point, because the lambda should have been destroyed + ASSERT_TRUE(called.unique()) << "Autowiring notification lambda was not properly cleaned up"; +} + From 451f5c89db1b1b1a3e7644812a02cc56a5633926 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Thu, 21 Aug 2014 18:17:16 -0700 Subject: [PATCH 2/4] Implemented unsafe solution. FIXME: There is a race condition in Autowire. PROBLEM: If the desired context member is added between the FindByTypeRecursive and AddDeferred calls then the notification will not be satisfied. --- autowiring/CoreContext.h | 19 +++++++++++++++++-- src/autowiring/CoreContext.cpp | 3 +++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/autowiring/CoreContext.h b/autowiring/CoreContext.h index f0810b88f..75c1a9683 100644 --- a/autowiring/CoreContext.h +++ b/autowiring/CoreContext.h @@ -130,7 +130,7 @@ class CoreContext: // This is a list of concrete types, indexed by the true type of each element. std::vector m_concreteTypes; - // This is a memoization map used to memoize any already-detected interfaces. The map + // This is a memoization map used to memoize any already-detected interfaces. mutable std::unordered_map m_typeMemos; // All known context members, exception filters: @@ -871,6 +871,10 @@ class CoreContext: /// template bool Autowire(AutowirableSlot& slot) { + // FIXME: This is subject to a race condition! + // If the desired member is added between FindByTypeRecursive + // and AddDeferred it will never be wired! + if(FindByTypeRecursive(slot)) return true; @@ -883,7 +887,8 @@ class CoreContext: /// Adds a post-attachment listener in this context for a particular autowired member /// /// - /// A pointer to a deferrable autowiring function which the caller may safely ignore if it's not needed + /// A pointer to a deferrable autowiring function which the caller may safely ignore if it's not needed. + /// Returns nullptr if the call was made immediately. /// /// /// This method will succeed if slot was constructed in this context or any parent context. If the @@ -902,6 +907,16 @@ class CoreContext: /// template const AutowirableSlotFn* NotifyWhenAutowired(Fn&& listener) { + /// GOAL: Check for satisfaction, return if satisfied, defer if not. + /// PROBLEM: UpdateDeferredElements only applies to NEW members. + /// PROBLEM: This needs to occur in a single atomic sequence. + + std::shared_ptr slot; + if (FindByTypeRecursive(slot)) { + listener(); + return nullptr; + } + auto retVal = MakeAutowirableSlotFn( shared_from_this(), std::forward(listener) diff --git a/src/autowiring/CoreContext.cpp b/src/autowiring/CoreContext.cpp index 011de19c1..a7acb60bc 100644 --- a/src/autowiring/CoreContext.cpp +++ b/src/autowiring/CoreContext.cpp @@ -481,6 +481,9 @@ void CoreContext::BuildCurrentState(void) { } void CoreContext::CancelAutowiringNotification(DeferrableAutowiring* pDeferrable) { + if (!pDeferrable) + return; + std::lock_guard lk(m_stateBlock->m_lock); auto q = m_typeMemos.find(pDeferrable->GetType()); if(q == m_typeMemos.end()) From 530dd136d7a733ffabe847ade78b235f8b019c5e Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Thu, 21 Aug 2014 19:21:11 -0700 Subject: [PATCH 3/4] Thread-correct (see file comments added in previous commit) NotifyWhenAutowired and Autowire methods. --- autowiring/CoreContext.h | 77 ++++++++++++++++++---------------- src/autowiring/CoreContext.cpp | 21 +++++++--- 2 files changed, 56 insertions(+), 42 deletions(-) diff --git a/autowiring/CoreContext.h b/autowiring/CoreContext.h index 75c1a9683..ee0cae5fb 100644 --- a/autowiring/CoreContext.h +++ b/autowiring/CoreContext.h @@ -7,6 +7,7 @@ #include "AutowiringEvents.h" #include "autowiring_error.h" #include "Bolt.h" +#include "CoreContextStateBlock.h" #include "CoreRunnable.h" #include "ContextMember.h" #include "CreationRules.h" @@ -29,7 +30,6 @@ #include STL_UNORDERED_MAP #include STL_UNORDERED_SET -struct CoreContextStateBlock; class AutoInjectable; class AutoPacketFactory; class DeferrableAutowiring; @@ -367,6 +367,11 @@ class CoreContext: /// void FindByTypeUnsafe(AnySharedPointer& reference) const; + /// + /// Recursive locking for Autowire satisfaction search + /// + void FindByTypeRecursiveUnsafe(AnySharedPointer& reference) const; + /// /// Returns or constructs a new AutoPacketFactory instance /// @@ -375,7 +380,7 @@ class CoreContext: /// /// Adds the specified deferrable autowiring as a general recipient of autowiring events /// - void AddDeferred(const AnySharedPointer& reference, DeferrableAutowiring* deferrable); + void AddDeferredUnsafe(const AnySharedPointer& reference, DeferrableAutowiring* deferrable); /// /// Adds a snooper to the snoopers set @@ -846,9 +851,9 @@ class CoreContext: /// template void FindByType(std::shared_ptr& slot) const { - AnySharedPointerT ptr; - FindByType(ptr); - slot = ptr.slot()->template as(); + AnySharedPointerT reference; + FindByType(reference); + slot = reference.slot()->template as(); } /// @@ -856,14 +861,13 @@ class CoreContext: /// template bool FindByTypeRecursive(std::shared_ptr& slot) { - // First-chance resolution in this context and ancestor contexts: - for(CoreContext* pCur = this; pCur; pCur = pCur->m_pParent.get()) { - pCur->FindByType(slot); - if(slot) - return true; + AnySharedPointerT reference; + { + std::lock_guard guard(m_stateBlock->m_lock); + FindByTypeRecursiveUnsafe(reference); } - - return false; + slot = reference.slot()->template as(); + return static_cast(slot); } /// @@ -871,16 +875,16 @@ class CoreContext: /// template bool Autowire(AutowirableSlot& slot) { - // FIXME: This is subject to a race condition! - // If the desired member is added between FindByTypeRecursive - // and AddDeferred it will never be wired! - - if(FindByTypeRecursive(slot)) - return true; - - // Failed, defer - AddDeferred(AnySharedPointerT(), &slot); - return false; + AnySharedPointerT reference; + { + std::lock_guard lk(m_stateBlock->m_lock); + FindByTypeRecursiveUnsafe(reference); + if (!slot) { + AddDeferredUnsafe(AnySharedPointerT(), &slot); + } + } + slot = reference.slot()->template as(); + return static_cast(slot); } /// @@ -907,22 +911,21 @@ class CoreContext: /// template const AutowirableSlotFn* NotifyWhenAutowired(Fn&& listener) { - /// GOAL: Check for satisfaction, return if satisfied, defer if not. - /// PROBLEM: UpdateDeferredElements only applies to NEW members. - /// PROBLEM: This needs to occur in a single atomic sequence. - - std::shared_ptr slot; - if (FindByTypeRecursive(slot)) { - listener(); - return nullptr; + AutowirableSlotFn* retVal = nullptr; + AnySharedPointerT asp; + { + std::lock_guard lk(m_stateBlock->m_lock); + FindByTypeRecursiveUnsafe(asp); + if (asp) { + listener(); + } else { + retVal = MakeAutowirableSlotFn( + shared_from_this(), + std::forward(listener) + ); + AddDeferredUnsafe(asp, retVal); + } } - - auto retVal = MakeAutowirableSlotFn( - shared_from_this(), - std::forward(listener) - ); - - AddDeferred(AnySharedPointerT(), retVal); return retVal; } diff --git a/src/autowiring/CoreContext.cpp b/src/autowiring/CoreContext.cpp index a7acb60bc..46b6a43bd 100644 --- a/src/autowiring/CoreContext.cpp +++ b/src/autowiring/CoreContext.cpp @@ -4,7 +4,6 @@ #include "AutoInjectable.h" #include "AutoPacketFactory.h" #include "BoltBase.h" -#include "CoreContextStateBlock.h" #include "CoreThread.h" #include "GlobalCoreContext.h" #include "JunctionBox.h" @@ -285,6 +284,21 @@ void CoreContext::FindByTypeUnsafe(AnySharedPointer& reference) const { m_typeMemos[type].m_value = reference; } +void CoreContext::FindByTypeRecursiveUnsafe(AnySharedPointer& reference) const { + FindByTypeUnsafe(reference); + if (reference) + // Type satisfied in current context + return; + + if (m_pParent) { + // Recurse while holding lock on this context + // NOTE: Deadlock is only possible if there is a simultaneous descending locked chain, + // but by definition of contexts this is forbidden. + std::lock_guard guard(m_pParent->m_stateBlock->m_lock); + m_pParent->FindByTypeRecursiveUnsafe(reference); + } +} + std::shared_ptr CoreContext::GetGlobal(void) { return std::static_pointer_cast(GlobalCoreContext::Get()); } @@ -767,10 +781,7 @@ std::shared_ptr CoreContext::GetPacketFactory(void) { return pf; } -void CoreContext::AddDeferred(const AnySharedPointer& reference, DeferrableAutowiring* deferrable) -{ - std::lock_guard lk(m_stateBlock->m_lock); - +void CoreContext::AddDeferredUnsafe(const AnySharedPointer& reference, DeferrableAutowiring* deferrable) { // Determine whether a type memo exists right now for the thing we're trying to defer. If it doesn't // exist, we need to inject one in order to allow deferred satisfaction to know what kind of type we // are trying to satisfy at this point. From b0ac22b981fbef99f3c9197a36d88337e83eb8ba Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Thu, 21 Aug 2014 20:09:16 -0700 Subject: [PATCH 4/4] Avoid missing type satisfaction due to simultaneous injection in to a parent context: FindByTypeRecursiveUnsafe makes calls while holding ALL necessary locks. --- autowiring/CoreContext.h | 51 +++++++++++++++++++--------------- src/autowiring/CoreContext.cpp | 13 ++++++--- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/autowiring/CoreContext.h b/autowiring/CoreContext.h index ee0cae5fb..25f6cf98e 100644 --- a/autowiring/CoreContext.h +++ b/autowiring/CoreContext.h @@ -25,8 +25,9 @@ #include "TypeUnifier.h" #include -#include TYPE_INDEX_HEADER #include MEMORY_HEADER +#include FUNCTIONAL_HEADER +#include TYPE_INDEX_HEADER #include STL_UNORDERED_MAP #include STL_UNORDERED_SET @@ -370,7 +371,10 @@ class CoreContext: /// /// Recursive locking for Autowire satisfaction search /// - void FindByTypeRecursiveUnsafe(AnySharedPointer& reference) const; + /// + /// The argument &&reference enables implicit type from AnySharedPointerT. + /// + void FindByTypeRecursiveUnsafe(AnySharedPointer&& reference, const std::function& terminal) const; /// /// Returns or constructs a new AutoPacketFactory instance @@ -861,12 +865,13 @@ class CoreContext: /// template bool FindByTypeRecursive(std::shared_ptr& slot) { - AnySharedPointerT reference; { std::lock_guard guard(m_stateBlock->m_lock); - FindByTypeRecursiveUnsafe(reference); + FindByTypeRecursiveUnsafe(AnySharedPointerT(), + [&slot](AnySharedPointer& reference){ + slot = reference.slot()->template as(); + }); } - slot = reference.slot()->template as(); return static_cast(slot); } @@ -875,15 +880,16 @@ class CoreContext: /// template bool Autowire(AutowirableSlot& slot) { - AnySharedPointerT reference; { std::lock_guard lk(m_stateBlock->m_lock); - FindByTypeRecursiveUnsafe(reference); - if (!slot) { - AddDeferredUnsafe(AnySharedPointerT(), &slot); - } + FindByTypeRecursiveUnsafe(AnySharedPointerT(), + [this, &slot](AnySharedPointer& reference){ + slot = reference.slot()->template as(); + if (!slot) { + AddDeferredUnsafe(AnySharedPointerT(), &slot); + } + }); } - slot = reference.slot()->template as(); return static_cast(slot); } @@ -912,19 +918,20 @@ class CoreContext: template const AutowirableSlotFn* NotifyWhenAutowired(Fn&& listener) { AutowirableSlotFn* retVal = nullptr; - AnySharedPointerT asp; { std::lock_guard lk(m_stateBlock->m_lock); - FindByTypeRecursiveUnsafe(asp); - if (asp) { - listener(); - } else { - retVal = MakeAutowirableSlotFn( - shared_from_this(), - std::forward(listener) - ); - AddDeferredUnsafe(asp, retVal); - } + FindByTypeRecursiveUnsafe(AnySharedPointerT(), + [this, &listener, &retVal](AnySharedPointer& reference) { + if (reference) { + listener(); + } else { + retVal = MakeAutowirableSlotFn( + shared_from_this(), + std::forward(listener) + ); + AddDeferredUnsafe(reference, retVal); + } + }); } return retVal; } diff --git a/src/autowiring/CoreContext.cpp b/src/autowiring/CoreContext.cpp index 46b6a43bd..50acfa938 100644 --- a/src/autowiring/CoreContext.cpp +++ b/src/autowiring/CoreContext.cpp @@ -284,18 +284,23 @@ void CoreContext::FindByTypeUnsafe(AnySharedPointer& reference) const { m_typeMemos[type].m_value = reference; } -void CoreContext::FindByTypeRecursiveUnsafe(AnySharedPointer& reference) const { +void CoreContext::FindByTypeRecursiveUnsafe(AnySharedPointer&& reference, const std::function& terminal) const { FindByTypeUnsafe(reference); - if (reference) + if (reference) { // Type satisfied in current context + terminal(reference); return; + } if (m_pParent) { + std::lock_guard guard(m_pParent->m_stateBlock->m_lock); // Recurse while holding lock on this context // NOTE: Deadlock is only possible if there is a simultaneous descending locked chain, // but by definition of contexts this is forbidden. - std::lock_guard guard(m_pParent->m_stateBlock->m_lock); - m_pParent->FindByTypeRecursiveUnsafe(reference); + m_pParent->FindByTypeRecursiveUnsafe(std::move(reference), terminal); + } else { + // Call function while holding all locks through global scope. + terminal(reference); } }