From dec223f0189867641ee335ce6db02b1733d44ec4 Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Tue, 10 Feb 2015 20:34:13 -0800 Subject: [PATCH 1/3] Correcting invocation of BasicThread::OnStop at synchronized level `BasicThread::OnStop` is meant to be invoked at passive level, and relies on this fact when making calls to other parts of the system. --- autowiring/CoreRunnable.h | 7 ++-- src/autowiring/BasicThread.cpp | 23 ++++++++++---- src/autowiring/test/CoreThreadTest.cpp | 44 ++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/autowiring/CoreRunnable.h b/autowiring/CoreRunnable.h index c586a39b4..f3139a35e 100644 --- a/autowiring/CoreRunnable.h +++ b/autowiring/CoreRunnable.h @@ -53,11 +53,12 @@ class CoreRunnable { /// Invoked by the base class Stop() method. Override this method to perform /// any needed cleanup. /// + /// + /// Specifies whether the runnable should stop normally or whether it should exit as quickly as possible. + /// /// - /// This method will be called at most once. + /// This method will be called at most once from a passive level. /// - /// Specifies whether the runnable should stop normally or whether - /// it should exit as quickly as possible. virtual void OnStop(bool graceful) {}; /// diff --git a/src/autowiring/BasicThread.cpp b/src/autowiring/BasicThread.cpp index 3b954c262..d7b859aed 100644 --- a/src/autowiring/BasicThread.cpp +++ b/src/autowiring/BasicThread.cpp @@ -5,6 +5,7 @@ #include "BasicThreadStateBlock.h" #include "ContextEnumerator.h" #include "fast_pointer_cast.h" +#include ATOMIC_HEADER BasicThread::BasicThread(const char* pName): ContextMember(pName), @@ -65,14 +66,13 @@ void BasicThread::DoRunLoopCleanup(std::shared_ptr&& ctxt, std::sha // Perform a manual notification of teardown listeners NotifyTeardownListeners(); - // Notify everyone that we're completed: - std::lock_guard lk(state->m_lock); + // Transition to stopped state. Synchronization not required, transitions are all one-way m_stop = true; - m_completed = true; m_running = false; - // No longer running, we MUST release the thread pointer to ensure proper teardown order - state->m_thisThread.detach(); + // Need to ensure that "stop" and "running" are actually updated in memory before we mark "complete" + std::atomic_thread_fence(std::memory_order_release); + m_completed = true; // Tell our CoreRunnable parent that we're done to ensure that our reference count will be cleared. Stop(false); @@ -86,8 +86,17 @@ void BasicThread::DoRunLoopCleanup(std::shared_ptr&& ctxt, std::sha // will destroy the entire underlying context. refTracker.reset(); - // Notify other threads that we are done. At this point, any held references that might - // still exist are held by entities other than ourselves. + // MUST detach here. By this point in the application, it's possible that `this` has already been + // deleted. If that's the case, `state.unique()` is true, and when we go out of scope, the destructor + // for m_thisThread will be invoked. If that happens, the destructor will block for the held thread + // to quit--and, in this case, the thread which is being held is actually us. Blocking on it, in that + // case, would be a trivial deadlock. So, because we're about to quit anyway, we simply detach the + // thread and prepare for final teardown operations. + state->m_thisThread.detach(); + + // Notify other threads that we are done. At this point, any held references that might still exist + // notification must happen from a synchronized level in order to ensure proper ordering. + std::lock_guard lk(state->m_lock); state->m_stateCondition.notify_all(); } diff --git a/src/autowiring/test/CoreThreadTest.cpp b/src/autowiring/test/CoreThreadTest.cpp index 685658bd5..78cfcf627 100644 --- a/src/autowiring/test/CoreThreadTest.cpp +++ b/src/autowiring/test/CoreThreadTest.cpp @@ -473,3 +473,47 @@ TEST_F(CoreThreadTest, VerifyThreadGetsOnStop) { EXPECT_TRUE(bsoosh->got_stopped) << "BasicThread instance did not receive an OnStop notification as expected"; EXPECT_TRUE(ctoosh->got_stopped) << "CoreThread instance did not receive an OnStop notification as expected"; } + +class MakesPassiveCallInOnStop: + public CoreThread +{ +public: + // Run operation is left empty, we want our thread to stop right after it starts + void Run(void) override {} + + bool bStartExcepted = false; + bool bStopExcepted = false; + + bool OnStart(void) override { + try { + std::lock_guard lk(GetLock()); + } + catch (...) { + bStartExcepted = true; + } + return CoreThread::OnStart(); + } + + void OnStop(void) override { + try { + // Causes an already-obtained exception if we are in rundown + std::lock_guard lk(GetLock()); + } + catch (...) { + bStopExcepted = true; + } + } +}; + +TEST_F(CoreThreadTest, LightsOutPassiveCall) { + AutoCurrentContext ctxt; + AutoRequired mpc; + ctxt->Initiate(); + + // Wait for bad things to happen + ASSERT_TRUE(mpc->WaitFor(std::chrono::seconds(5))) << "Passive call thread took too long to quit"; + + // Verify no bad things happened: + ASSERT_FALSE(mpc->bStartExcepted) << "Exception occurred during an attempt to elevate to synchronized level from CoreThread::OnStart"; + ASSERT_FALSE(mpc->bStopExcepted) << "Exception occurred during an attempt to elevate to synchronized level from CoreThread::OnStop"; +} \ No newline at end of file From c823d68f15479b57c567b13bf58b236181e5f37c Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Tue, 10 Feb 2015 22:05:42 -0800 Subject: [PATCH 2/3] Eliminating deprecated RemoveSatCounter routine Routine was last used back when `ObjectPool` was being used to pool `AutoPacket` instances; `AutoPacket::Initialize` was being used to restore the packet to its default state. --- autowiring/AutoPacket.h | 5 ----- src/autowiring/AutoPacket.cpp | 20 -------------------- 2 files changed, 25 deletions(-) diff --git a/autowiring/AutoPacket.h b/autowiring/AutoPacket.h index f5034e5c4..5831a3aea 100644 --- a/autowiring/AutoPacket.h +++ b/autowiring/AutoPacket.h @@ -90,11 +90,6 @@ class AutoPacket: /// void AddSatCounter(SatCounter& satCounter); - /// - /// Removes all AutoFilter argument information for a recipient - /// - void RemoveSatCounter(const SatCounter& satCounter); - /// /// Marks the specified entry as being unsatisfiable /// diff --git a/src/autowiring/AutoPacket.cpp b/src/autowiring/AutoPacket.cpp index 271f66e61..71e754157 100644 --- a/src/autowiring/AutoPacket.cpp +++ b/src/autowiring/AutoPacket.cpp @@ -121,26 +121,6 @@ void AutoPacket::AddSatCounter(SatCounter& satCounter) { } } -void AutoPacket::RemoveSatCounter(const SatCounter& satCounter) { - for(auto pCur = satCounter.GetAutoFilterInput(); *pCur; pCur++) { - DecorationKey key(*pCur->ti, pCur->tshift); - - DecorationDisposition* entry = &m_decorations[key]; - - // Decide what to do with this entry: - if (pCur->is_input) { - assert(!entry->m_subscribers.empty()); - assert(&satCounter == entry->m_subscribers.back()); - entry->m_subscribers.pop_back(); - } - if (pCur->is_output) { - assert(!entry->m_publishers.empty()); - assert(&satCounter == entry->m_publishers.back()); - entry->m_publishers.pop_back(); - } - } -} - void AutoPacket::MarkUnsatisfiable(const DecorationKey& key) { // Perform unsatisfaction logic if (key.tshift) { From c4ebf5a7f925617eabfa2e2dcfcdb9d9fd092671 Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Wed, 11 Feb 2015 06:26:03 -0800 Subject: [PATCH 3/3] Comment fixups Very small changes to address some formatting inconsistencies in comments noticed while working on an unrelated pull request. --- autowiring/CoreContext.h | 4 ++++ autowiring/CoreRunnable.h | 21 +++++++++++---------- autowiring/DispatchQueue.h | 2 ++ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/autowiring/CoreContext.h b/autowiring/CoreContext.h index f789cab6b..3b8419c33 100644 --- a/autowiring/CoreContext.h +++ b/autowiring/CoreContext.h @@ -521,10 +521,14 @@ class CoreContext: template bool Is(void) const { return GetSigilType() == typeid(Sigil); } + /// /// The first child in the set of this context's children. + /// std::shared_ptr FirstChild(void) const; + /// /// The next context sharing the same parent, or null if this is the last entry in the list + /// std::shared_ptr NextSibling(void) const; /// diff --git a/autowiring/CoreRunnable.h b/autowiring/CoreRunnable.h index c586a39b4..16827651d 100644 --- a/autowiring/CoreRunnable.h +++ b/autowiring/CoreRunnable.h @@ -38,26 +38,27 @@ class CoreRunnable { const std::shared_ptr& GetOutstanding(void) const; /// - /// Invoked by the Start() method. Override this method to perform - /// any needed setup; + /// Invoked by the Start() method. Override this method to perform any needed setup /// - /// True if processing has started, false otherwise. When overriding, returning false - /// will shut down the runnable immediately. + /// + /// True if processing has started, false otherwise. When overriding, returning false will shut down the + /// runnable immediately. + /// /// - /// This method will be called at most once. Returning false from this method will result in an immediate invocation - /// of the OnStop(false). + /// This method will be called at most once. Returning false from this method will result in an immediate + /// invocation of OnStop(false). /// virtual bool OnStart(void) { return false; }; /// - /// Invoked by the base class Stop() method. Override this method to perform - /// any needed cleanup. + /// Invoked by the base class Stop() method. Override this method to perform any needed cleanup. /// /// /// This method will be called at most once. /// - /// Specifies whether the runnable should stop normally or whether - /// it should exit as quickly as possible. + /// + /// Specifies whether the runnable should stop normally or whether it should exit as quickly as possible. + /// virtual void OnStop(bool graceful) {}; /// diff --git a/autowiring/DispatchQueue.h b/autowiring/DispatchQueue.h index 02aa1d522..c76b14e54 100644 --- a/autowiring/DispatchQueue.h +++ b/autowiring/DispatchQueue.h @@ -56,6 +56,8 @@ class DispatchQueue { // Notice when the dispatch queue has been updated: std::condition_variable m_queueUpdated; + // True if DispatchQueue::Abort has been called. This will cause the dispatch queue's remaining entries + // to be dumped and prevent the introduction of new entries to the queue. bool m_aborted; ///