Fix signals cyclic unlink issue

autowiring/AutowirableSlot.h

@@ -108,7 +108,7 @@ class DeferrableAutowiring
   /// This method may not be safely called from an unsynchronized context.  Callers must ensure that
   /// this field is not in use during the call to reset or a data race will result.
   /// </remarks>
-  void reset();
+  virtual void reset(void);
 
   /// <returns>
   /// The strategy that should be used to satisfy this slot
@@ -119,15 +119,20 @@ class DeferrableAutowiring
   virtual DeferrableUnsynchronizedStrategy* GetStrategy(void) { return nullptr; }
 
   /// <summary>
+  /// Releases autowiring objects that are associated with the current slot
   /// </summary>
+  /// <remarks>
+  /// These objects are generally all objects that have been registered on this slot to receive notifications of
+  /// some kind when the slot itself is satisfied.  When the context is tearing down, there is potentially an
+  /// ownership and responsibility race on this datatype--should the dependant chain be executed on the objects
+  /// that have been satisfied, or should it be destroyed?
+  /// </remarks>
   virtual DeferrableAutowiring* ReleaseDependentChain(void) { return nullptr; }
 
   /// <summary>
   /// Satisfies autowiring with a so-called "witness slot" which is guaranteed to be satisfied on the same type
   /// </summary>
-  void SatisfyAutowiring(const AnySharedPointer& ptr) {
-    m_ptr = ptr;
-  }
+  void SatisfyAutowiring(const AnySharedPointer& ptr);
 
   bool operator!=(const AnySharedPointer& rhs) const { return m_ptr != rhs; }
   bool operator==(const AnySharedPointer& rhs) const { return m_ptr == rhs; }

autowiring/Autowired.h

@@ -116,23 +116,7 @@ class Autowired:
   }
 
   ~Autowired(void) {
-    // And remove any events that were added to the object by this autowired field
-    auto localEvents = std::move(m_events);
-    for (auto& registration : localEvents) {
-      registration.reset();
-    }
-
-    if(m_pFirstChild == this)
-      // Tombstoned, nothing to do:
-      return;
-
-    // Need to ensure that nobody tries to fill us while we are tearing down:
-    this->CancelAutowiring();
-
-    // And now we destroy our deferrable autowiring collection:
-    std::unique_ptr<DeferrableAutowiring> prior;
-    for(DeferrableAutowiring* cur = m_pFirstChild; cur; cur = cur->GetFlink())
-      prior.reset(cur);
+    reset();
   }
 
 private:
@@ -185,14 +169,41 @@ class Autowired:
     return signal_relay<U, Args... >(*this, sig);
   }
 
+  // AutowirableSlot overrides:
+  void reset(void) override {
+    // And remove any events that were added to the object by this autowired field
+    auto localEvents = std::move(m_events);
+    for (auto& registration : localEvents)
+      registration.reset();
+
+    // Linked list unwind deletion:
+    for (
+      DeferrableAutowiring *prior, *cur = ReleaseDependentChain();
+      cur;
+      cur = cur->GetFlink(), delete prior
+    )
+      prior = cur;
+
+    // Base type completes the reset behavior
+    AutowirableSlot<T>::reset();
+  }
+
   /// <summary>
   /// Assigns a lambda function to be called when the dependency for this slot is autowired.
   /// </summary>
   /// <remarks>
   /// In contrast with CoreContext::NotifyWhenAutowired, the specified lambda is only
   /// called as long as this Autowired slot has not been destroyed.  If this slot is destroyed
   /// before the dependency is satisfied, i.e. because the owning context shuts down, the
-  /// lambda is never invoked.
+  /// lambda is cancelled.
+  ///
+  /// Note that, if T is injected at about the same time as this object is destroyed, then cancellation
+  /// of the autowired lambda could potentially race with satisfaction of that same lambda.  This type
+  /// of race is called a cancellation race, and can result in the lambda being invoked even though this
+  /// object has been destroyed.
+  ///
+  /// Users who use Autowired as a member of their class do not need to consider this case.  Autowired
+  /// fields declared as class members are always cancelled before the enclosing object is destroyed.
   ///
   /// \include snippets/Autowired_Notify.txt
   /// </remarks>

src/autowiring/AutowirableSlot.cpp

@@ -37,3 +37,7 @@ void DeferrableAutowiring::CancelAutowiring(void) {
   // Tell our context we are going away:
   context->CancelAutowiringNotification(this);
 }
+
+void DeferrableAutowiring::SatisfyAutowiring(const AnySharedPointer& ptr) {
+  m_ptr = ptr;
+}

src/autowiring/test/AutoSignalTest.cpp

@@ -387,4 +387,46 @@ TEST_F(AutoSignalTest, CanMoveSignal) {
 
   signal();
   ASSERT_TRUE(hit) << "Registered listeners were not correctly moved under move assignment";
+}
+
+namespace {
+  class ObjectA;
+  class ObjectB;
+
+  class ObjectA {
+  public:
+    ObjectA(void);
+
+    autowiring::signal<void()> x;
+    Autowired<ObjectB> b;
+  };
+
+  class ObjectB {
+  public:
+    ObjectB(void);
+
+    autowiring::signal<void()> y;
+    Autowired<ObjectA> a;
+  };
+
+  ObjectA::ObjectA(void) {
+    b(&ObjectB::y) += [] {};
+  }
+
+  ObjectB::ObjectB(void) {
+    a(&ObjectA::x) += [] {};
+  }
+}
+
+TEST_F(AutoSignalTest, CyclicRegistrationUnlink) {
+  std::weak_ptr<CoreContext> ctxt;
+  {
+    AutoCreateContext ctxt;
+    ctxt->SetUnlinkOnTeardown(true);
+
+    AutoRequired<ObjectA> a(ctxt);
+    AutoRequired<ObjectB> b(ctxt);
+  }
+
+  ASSERT_TRUE(ctxt.expired()) << "A context pointer was linked in an intentional cyclic dependency network";
 }