From 9177ae5e43cb4aacce651bb38c32f5200e45d798 Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Tue, 11 Aug 2015 12:28:17 -0700 Subject: [PATCH] Fix signals cyclic unlink issue Add unit test to show regression in cyclic unlink countercase. This occurs when a signal handler is registered on an Autowired field which is unlinked by CoreContext. --- autowiring/AutowirableSlot.h | 13 ++++--- autowiring/Autowired.h | 47 ++++++++++++++++---------- src/autowiring/AutowirableSlot.cpp | 4 +++ src/autowiring/test/AutoSignalTest.cpp | 42 +++++++++++++++++++++++ 4 files changed, 84 insertions(+), 22 deletions(-) diff --git a/autowiring/AutowirableSlot.h b/autowiring/AutowirableSlot.h index 0d649643b..63df6e3de 100644 --- a/autowiring/AutowirableSlot.h +++ b/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. /// - void reset(); + virtual void reset(void); /// /// The strategy that should be used to satisfy this slot @@ -119,15 +119,20 @@ class DeferrableAutowiring virtual DeferrableUnsynchronizedStrategy* GetStrategy(void) { return nullptr; } /// + /// Releases autowiring objects that are associated with the current slot /// + /// + /// 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? + /// virtual DeferrableAutowiring* ReleaseDependentChain(void) { return nullptr; } /// /// Satisfies autowiring with a so-called "witness slot" which is guaranteed to be satisfied on the same type /// - 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; } diff --git a/autowiring/Autowired.h b/autowiring/Autowired.h index 9df7c80cc..bcdfd8bcf 100644 --- a/autowiring/Autowired.h +++ b/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 prior; - for(DeferrableAutowiring* cur = m_pFirstChild; cur; cur = cur->GetFlink()) - prior.reset(cur); + reset(); } private: @@ -185,6 +169,25 @@ class Autowired: return signal_relay(*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::reset(); + } + /// /// Assigns a lambda function to be called when the dependency for this slot is autowired. /// @@ -192,7 +195,15 @@ class Autowired: /// 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 /// diff --git a/src/autowiring/AutowirableSlot.cpp b/src/autowiring/AutowirableSlot.cpp index b3ec33002..b8b2d5805 100644 --- a/src/autowiring/AutowirableSlot.cpp +++ b/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; +} diff --git a/src/autowiring/test/AutoSignalTest.cpp b/src/autowiring/test/AutoSignalTest.cpp index bc92589f1..344355cc7 100644 --- a/src/autowiring/test/AutoSignalTest.cpp +++ b/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 x; + Autowired b; + }; + + class ObjectB { + public: + ObjectB(void); + + autowiring::signal y; + Autowired a; + }; + + ObjectA::ObjectA(void) { + b(&ObjectB::y) += [] {}; + } + + ObjectB::ObjectB(void) { + a(&ObjectA::x) += [] {}; + } +} + +TEST_F(AutoSignalTest, CyclicRegistrationUnlink) { + std::weak_ptr ctxt; + { + AutoCreateContext ctxt; + ctxt->SetUnlinkOnTeardown(true); + + AutoRequired a(ctxt); + AutoRequired b(ctxt); + } + + ASSERT_TRUE(ctxt.expired()) << "A context pointer was linked in an intentional cyclic dependency network"; } \ No newline at end of file