Merge branch 'develop'

autowiring/AutoPacket.h

@@ -90,11 +90,6 @@ class AutoPacket:
   /// </summary>
   void AddSatCounter(SatCounter& satCounter);
 
-  /// <summary>
-  /// Removes all AutoFilter argument information for a recipient
-  /// </summary>
-  void RemoveSatCounter(const SatCounter& satCounter);
-
   /// <summary>
   /// Marks the specified entry as being unsatisfiable
   /// </summary>

autowiring/CoreContext.h

@@ -521,10 +521,14 @@ class CoreContext:
   template<class Sigil>
   bool Is(void) const { return GetSigilType() == typeid(Sigil); }
 
+  /// <summary>
   /// The first child in the set of this context's children.
+  /// </summary>
   std::shared_ptr<CoreContext> FirstChild(void) const;
 
+  /// <summary>
   /// The next context sharing the same parent, or null if this is the last entry in the list
+  /// </summary>
   std::shared_ptr<CoreContext> NextSibling(void) const;
 
   /// <summary>

autowiring/CoreRunnable.h

@@ -38,26 +38,27 @@ class CoreRunnable {
   const std::shared_ptr<CoreObject>& GetOutstanding(void) const;
 
   /// <summary>
-  /// 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
   /// </summary>
-  /// <returns>True if processing has started, false otherwise. When overriding, returning false
-  /// will shut down the runnable immediately.</returns>
+  /// <returns>
+  /// True if processing has started, false otherwise. When overriding, returning false will shut down the
+  /// runnable immediately.
+  /// </returns>
   /// <remarks>
-  /// 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).
   /// </remarks>
   virtual bool OnStart(void) { return false; };
 
   /// <summary>
-  /// 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.
   /// </summary>
+  /// <param name="graceful">
+  /// Specifies whether the runnable should stop normally or whether it should exit as quickly as possible.
+  /// </param>
   /// <remarks>
-  /// This method will be called at most once. 
+  /// This method will be called at most once from a passive level.
   /// </remarks>
-  /// <param name="graceful">Specifies whether the runnable should stop normally or whether
-  /// it should exit as quickly as possible.</param>
   virtual void OnStop(bool graceful) {};
 
   /// <summary>

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;
 
   /// <summary>

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) {

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<CoreContext>&& ctxt, std::sha
   // Perform a manual notification of teardown listeners
   NotifyTeardownListeners();
 
-  // Notify everyone that we're completed:
-  std::lock_guard<std::mutex> 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<CoreContext>&& 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<std::mutex> lk(state->m_lock);
   state->m_stateCondition.notify_all();
 }
 

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<std::mutex> 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<std::mutex> lk(GetLock());
+    }
+    catch (...) {
+      bStopExcepted = true;
+    }
+  }
+};
+
+TEST_F(CoreThreadTest, LightsOutPassiveCall) {
+  AutoCurrentContext ctxt;
+  AutoRequired<MakesPassiveCallInOnStop> 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";
+}