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/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..63ee78e3f 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. /// + /// + /// 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/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; /// 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) { 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