Merge pull request #505 from leapmotion/bug-creatorteardown

Fix bug in ContextCreator Clear
leapmotion · May 15, 2015 · 64e4d0b · 64e4d0b
2 parents 93ee79f + b00e1a3
commit 64e4d0b
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 25 deletions.
diff --git a/autowiring/ContextCreatorBase.h b/autowiring/ContextCreatorBase.h
@@ -24,38 +24,26 @@ class ContextCreatorBase {
   /// <param name="ctr">The collection of contexts to be cleared</param>
   /// <param name="iter">An iterator locker, which can obtain a shared pointer from an iterator</param>
   /// <remarks>
-  /// This method is synchronized on the contextLock and this will generally make it thread-safe
+  /// This method is synchronized on the contextLock and this will generally make it thread-safe.
+  /// It will clear the local context list immediately, but it is not garunteed that the context
+  /// list will be empty at the time of return.
   /// </remarks>
   template<class Ctr, class Fx>
   void Clear(bool wait, Ctr& ctr, Fx&& locker) {
-    if(!wait) {
-      // Trivial signal-clear-return:
-      std::lock_guard<std::mutex> lk(m_contextLock);
-      for(const auto& ctxt : ctr) {
-        auto locked = locker(ctxt);
-        if(locked)
-          locked->SignalShutdown();
-      }
-      ctr.clear();
-      return;
+    // Move out under lock
+    Ctr ctrLocal = (std::lock_guard<std::mutex>(m_contextLock), std::move(ctr));
+
+    for (const auto& ctxt : ctrLocal) {
+      auto locked = locker(ctxt);
+      if(locked)
+        locked->SignalShutdown();
     }
 
-    Ctr ctrCopy;
-
-    // Copy out and clear:
-    {
-      std::lock_guard<std::mutex> lk(m_contextLock);
-      for(const auto& ctxt : ctr) {
-        auto locked = locker(ctxt);
-        if(locked)
-          locked->SignalShutdown();
-      }
-      ctrCopy = ctr;
-      ctr.clear();
-    }
+    if (!wait)
+      return;
 
     // Signal everyone first, then wait in a second pass:
-    for(const auto& ctxt : ctrCopy) {
+    for (const auto& ctxt : ctrLocal) {
       auto locked = locker(ctxt);
       if(locked)
         locked->Wait();

diff --git a/src/autowiring/test/ContextCreatorTest.cpp b/src/autowiring/test/ContextCreatorTest.cpp
@@ -200,3 +200,27 @@ TEST_F(ContextCreatorTest, VoidKeyType) {
   EXPECT_EQ(0UL, vc->GetSize()) << "A void context creator was not correctly updated when its dependent context went out of scope";
   EXPECT_EQ(2UL, vc->m_totalDestroyed) << "The void creator did not receive the expected number of NotifyContextDestroyed calls";
 }
+
+struct mySigil {};
+
+class Runnable:
+  public CoreRunnable
+{
+public:
+  bool OnStart(void) override { return true;  }
+};
+
+TEST_F(ContextCreatorTest, TeardownListenerTest) {
+  // Create a context and verify teardown happens as expected
+  AutoCreateContext mainContext;
+  CurrentContextPusher pusher(mainContext);
+
+  AutoRequired<ContextCreator<mySigil, int>> creator;
+  {
+    auto subctxt = creator->CreateContext(0).first;
+    auto brc = subctxt->Inject<Runnable>();
+    subctxt->Initiate();
+  }
+  creator->Clear(true);
+  ASSERT_TRUE(true) << "Really all this test has to do is not crash by this point.";
+}