Recursively call CoreContext::Stop before waiting on anything

This change ensures that all members of the current context receive an `OnStop` message before a wait is triggered on child contexts.

It's also gradually becoming clear that it may be necessary to deprecate `SignalShutdown(true)` as a typical calling convention.  We should probably require users to manually call `Wait` if that's what they want to do.

src/autowiring/CoreContext.cpp

@@ -504,57 +504,48 @@ void CoreContext::SignalShutdown(bool wait, ShutdownMode shutdownMode) {
     m_stateBlock->m_stateChanged.notify_all();
   }
 
+  // Teardown interleave assurance--all of these contexts will generally be destroyed
+  // at the exit of this block, due to the behavior of SignalTerminate, unless exterior
+  // context references (IE, related to snooping) exist.
+  //
+  // This is done in order to provide a stable collection that may be traversed during
+  // teardown outside of a lock.
+  std::vector<std::shared_ptr<CoreContext>> childrenInterleave;
+
   {
-    // Teardown interleave assurance--all of these contexts will generally be destroyed
-    // at the exit of this block, due to the behavior of SignalTerminate, unless exterior
-    // context references (IE, related to snooping) exist.
-    //
-    // This is done in order to provide a stable collection that may be traversed during
-    // teardown outside of a lock.
-    std::vector<std::shared_ptr<CoreContext>> childrenInterleave;
+    // Tear down all the children.
+    std::lock_guard<std::mutex> lk(m_stateBlock->m_lock);
 
-    {
-      // Tear down all the children.
-      std::lock_guard<std::mutex> lk(m_stateBlock->m_lock);
+    // Fill strong lock series in order to ensure proper teardown interleave:
+    childrenInterleave.reserve(m_children.size());
+    for(const auto& entry : m_children) {
+      auto childContext = entry.lock();
+
+      // Technically, it *is* possible for this weak pointer to be expired, even though
+      // we're holding the lock.  This may happen if the context itself is exiting even
+      // as we are processing SignalTerminate.  In that case, the child context in
+      // question is blocking in its dtor lambda, waiting patiently until we're done,
+      // at which point it will modify the m_children collection.
+      if(!childContext)
+        continue;
 
-      // Fill strong lock series in order to ensure proper teardown interleave:
-      childrenInterleave.reserve(m_children.size());
-      for(const auto& entry : m_children) {
-        auto childContext = entry.lock();
-
-        // Technically, it *is* possible for this weak pointer to be expired, even though
-        // we're holding the lock.  This may happen if the context itself is exiting even
-        // as we are processing SignalTerminate.  In that case, the child context in
-        // question is blocking in its dtor lambda, waiting patiently until we're done,
-        // at which point it will modify the m_children collection.
-        if(!childContext)
-          continue;
-
-        // Add to the interleave so we can SignalTerminate in a controlled way.
-        childrenInterleave.push_back(childContext);
-      }
+      // Add to the interleave so we can SignalTerminate in a controlled way.
+      childrenInterleave.push_back(childContext);
     }
-
-    // Now that we have a locked-down, immutable series, begin termination signalling:
-    for(size_t i = childrenInterleave.size(); i--; )
-      childrenInterleave[i]->SignalShutdown(wait);
   }
 
+  // Now that we have a locked-down, immutable series, begin termination signalling:
+  for(size_t i = childrenInterleave.size(); i--; )
+    childrenInterleave[i]->SignalShutdown(false, shutdownMode);
+
   // Pass notice to all child threads:
   bool graceful = (shutdownMode == ShutdownMode::Graceful);
   for (auto itr = firstThreadToStop; itr != m_threads.end(); ++itr)
     (*itr)->Stop(graceful);
 
-  // Signal our condition variable
-  m_stateBlock->m_stateChanged.notify_all();
-
-  // Short-return if required:
-  if(!wait)
-    return;
-
-  // Wait for the treads to finish before returning.
-  for (CoreRunnable* runnable : m_threads)
-    runnable->Wait();
+  // Wait if requested
+  if(wait)
+    Wait();
 }
 
 void CoreContext::Wait(void) {

src/autowiring/test/CoreContextTest.cpp

@@ -360,4 +360,79 @@ TEST_F(CoreContextTest, CoreContextAdd) {
 
   Autowired<CoreContextAddTestClass> mc;
   ASSERT_TRUE(mc.IsAutowired()) << "Manually registered interface was not detected as expected";
-}
+}
+
+struct ExplicitlyHoldsOutstandingCount:
+  public CoreRunnable
+{
+public:
+  bool OnStart(void) override {
+    outstanding = CoreRunnable::GetOutstanding();
+    return true;
+  }
+
+  void OnStop(bool) override {
+    // Just mark that this event took place
+    calledStopPrimitive = true;
+    calledStop.set_value(true);
+  }
+
+  void DoAdditionalWait(void) override {
+    std::unique_lock<std::mutex> lk(lock);
+    cv.wait(lk, [&] { return !outstanding; });
+  }
+
+  void Proceed(void) {
+    std::lock_guard<std::mutex> lk(lock);
+    outstanding.reset();
+    cv.notify_all();
+  }
+
+  bool calledStopPrimitive = false;
+  std::promise<bool> calledStop;
+
+  std::mutex lock;
+  std::condition_variable cv;
+  std::shared_ptr<CoreObject> outstanding;
+};
+
+TEST_F(CoreContextTest, AppropriateShutdownInterleave) {
+  // Need both an outer and an inner context
+  AutoCurrentContext ctxtOuter;
+  AutoCreateContext ctxtInner;
+
+  // Need to inject types at both scopes
+  AutoRequired<ExplicitlyHoldsOutstandingCount> outer(ctxtOuter);
+  AutoRequired<ExplicitlyHoldsOutstandingCount> inner(ctxtInner);
+
+  // Start both contexts up
+  ctxtOuter->Initiate();
+  ctxtInner->Initiate();
+
+  // Now shut down the outer context.  Hand off to an async, we want this to block.
+  std::future<void> holder = std::async(
+    std::launch::async,
+    [&] {
+      ctxtOuter->SignalShutdown(true);
+    }
+  );
+
+  // Need to ensure that both outstanding counters are reset at some point:
+  {
+    auto cleanup = MakeAtExit([&] {
+      outer->Proceed();
+      inner->Proceed();
+    });
+
+    // Outer entry should have called "stop":
+    auto future = outer->calledStop.get_future();
+    ASSERT_EQ(
+      std::future_status::ready,
+      future.wait_for(std::chrono::seconds(5))
+    ) << "Outer scope's OnStop method was incorrectly blocked by a child context member taking a long time to shut down, " << outer->calledStopPrimitive;
+  }
+
+  // Both contexts should be stopped now:
+  ASSERT_TRUE(ctxtOuter->Wait(std::chrono::seconds(5))) << "Outer context did not tear down in a timely fashion";
+  ASSERT_TRUE(ctxtInner->Wait(std::chrono::seconds(5))) << "Inner context did not tear down in a timely fashion";
+}