Merge pull request #77 from GabrielHare/bug-context-notify-posthoc

Bug context notify posthoc
leapmotion · Aug 22, 2014 · 0e3c0de · 0e3c0de
2 parents 8d95603 + b0ac22b
commit 0e3c0de
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 35 deletions.
diff --git 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"
@@ -24,12 +25,12 @@
 #include "TypeUnifier.h"
 
 #include <list>
-#include TYPE_INDEX_HEADER
 #include MEMORY_HEADER
+#include FUNCTIONAL_HEADER
+#include TYPE_INDEX_HEADER
 #include STL_UNORDERED_MAP
 #include STL_UNORDERED_SET
 
-struct CoreContextStateBlock;
 class AutoInjectable;
 class AutoPacketFactory;
 class DeferrableAutowiring;
@@ -130,7 +131,7 @@ class CoreContext:
   // This is a list of concrete types, indexed by the true type of each element.
   std::vector<AnySharedPointer> 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<std::type_index, MemoEntry> m_typeMemos;
 
   // All known context members, exception filters:
@@ -367,6 +368,14 @@ class CoreContext:
   /// </summary>
   void FindByTypeUnsafe(AnySharedPointer& reference) const;
 
+  /// <summary>
+  /// Recursive locking for Autowire satisfaction search
+  /// </summary>
+  /// <remarks>
+  /// The argument &&reference enables implicit type from AnySharedPointerT<T>.
+  /// </remarks>
+  void FindByTypeRecursiveUnsafe(AnySharedPointer&& reference, const std::function<void(AnySharedPointer&)>& terminal) const;
+
   /// <summary>
   /// Returns or constructs a new AutoPacketFactory instance
   /// </summary>
@@ -375,7 +384,7 @@ class CoreContext:
   /// <summary>
   /// Adds the specified deferrable autowiring as a general recipient of autowiring events
   /// </summary>
-  void AddDeferred(const AnySharedPointer& reference, DeferrableAutowiring* deferrable);
+  void AddDeferredUnsafe(const AnySharedPointer& reference, DeferrableAutowiring* deferrable);
 
   /// <summary>
   /// Adds a snooper to the snoopers set
@@ -846,44 +855,50 @@ class CoreContext:
   /// </summary>
   template<class T>
   void FindByType(std::shared_ptr<T>& slot) const {
-    AnySharedPointerT<T> ptr;
-    FindByType(ptr);
-    slot = ptr.slot()->template as<T>();
+    AnySharedPointerT<T> reference;
+    FindByType(reference);
+    slot = reference.slot()->template as<T>();
   }
 
   /// <summary>
   /// Identical to Autowire, but will not register the passed slot for deferred resolution
   /// </summary>
   template<class T>
   bool FindByTypeRecursive(std::shared_ptr<T>& 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;
+    {
+      std::lock_guard<std::mutex> guard(m_stateBlock->m_lock);
+      FindByTypeRecursiveUnsafe(AnySharedPointerT<T>(),
+      [&slot](AnySharedPointer& reference){
+        slot = reference.slot()->template as<T>();
+      });
     }
-
-    return false;
+    return static_cast<bool>(slot);
   }
 
   /// <summary>
   /// Registers a slot to be autowired
   /// </summary>
   template<class T>
   bool Autowire(AutowirableSlot<T>& slot) {
-    if(FindByTypeRecursive(slot))
-      return true;
-
-    // Failed, defer
-    AddDeferred(AnySharedPointerT<T>(), &slot);
-    return false;
+    {
+      std::lock_guard<std::mutex> lk(m_stateBlock->m_lock);
+      FindByTypeRecursiveUnsafe(AnySharedPointerT<T>(),
+      [this, &slot](AnySharedPointer& reference){
+        slot = reference.slot()->template as<T>();
+        if (!slot) {
+          AddDeferredUnsafe(AnySharedPointerT<T>(), &slot);
+        }
+      });
+    }
+    return static_cast<bool>(slot);
   }
 
   /// <summary>
   /// Adds a post-attachment listener in this context for a particular autowired member
   /// </summary>
   /// <returns>
-  /// 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.
   /// </returns>
   /// <remarks>
   /// This method will succeed if slot was constructed in this context or any parent context.  If the
@@ -902,12 +917,22 @@ class CoreContext:
   /// </remarks>
   template<class T, class Fn>
   const AutowirableSlotFn<T, Fn>* NotifyWhenAutowired(Fn&& listener) {
-    auto retVal = MakeAutowirableSlotFn<T>(
-      shared_from_this(),
-      std::forward<Fn>(listener)
-    );
-
-    AddDeferred(AnySharedPointerT<T>(), retVal);
+    AutowirableSlotFn<T, Fn>* retVal = nullptr;
+    {
+      std::lock_guard<std::mutex> lk(m_stateBlock->m_lock);
+      FindByTypeRecursiveUnsafe(AnySharedPointerT<T>(),
+      [this, &listener, &retVal](AnySharedPointer& reference) {
+        if (reference) {
+          listener();
+        } else {
+          retVal = MakeAutowirableSlotFn<T>(
+            shared_from_this(),
+            std::forward<Fn>(listener)
+          );
+          AddDeferredUnsafe(reference, retVal);
+        }
+      });
+    }
     return retVal;
   }
 

diff --git 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,26 @@ void CoreContext::FindByTypeUnsafe(AnySharedPointer& reference) const {
   m_typeMemos[type].m_value = reference;
 }
 
+void CoreContext::FindByTypeRecursiveUnsafe(AnySharedPointer&& reference, const std::function<void(AnySharedPointer&)>& terminal) const {
+  FindByTypeUnsafe(reference);
+  if (reference) {
+    // Type satisfied in current context
+    terminal(reference);
+    return;
+  }
+
+  if (m_pParent) {
+    std::lock_guard<std::mutex> 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.
+    m_pParent->FindByTypeRecursiveUnsafe(std::move(reference), terminal);
+  } else {
+    // Call function while holding all locks through global scope.
+    terminal(reference);
+  }
+}
+
 std::shared_ptr<CoreContext> CoreContext::GetGlobal(void) {
   return std::static_pointer_cast<CoreContext, GlobalCoreContext>(GlobalCoreContext::Get());
 }
@@ -481,6 +500,9 @@ void CoreContext::BuildCurrentState(void) {
 }
 
 void CoreContext::CancelAutowiringNotification(DeferrableAutowiring* pDeferrable) {
+  if (!pDeferrable)
+    return;
+
   std::lock_guard<std::mutex> lk(m_stateBlock->m_lock);
   auto q = m_typeMemos.find(pDeferrable->GetType());
   if(q == m_typeMemos.end())
@@ -764,10 +786,7 @@ std::shared_ptr<AutoPacketFactory> CoreContext::GetPacketFactory(void) {
   return pf;
 }
 
-void CoreContext::AddDeferred(const AnySharedPointer& reference, DeferrableAutowiring* deferrable)
-{
-  std::lock_guard<std::mutex> 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.

diff --git 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<SimpleObject>(
-    [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<bool>(false);
+  AutoCurrentContext ctxt;
+
+  // Create an object that will satisfy subsequent notification call:
+  AutoRequired<SimpleObject> sobj;
+
+  // Notification should be immediate:
+  ctxt->NotifyWhenAutowired<SimpleObject>(
+  [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";
+}
+