Merge pull request #211 from CppMicroServices/210-bundle-thread-deadlock

Prevent possible deadlock during framework shutdown.

framework/src/bundle/BundleThread.cpp

@@ -41,13 +41,12 @@ const int BundleThread::OP_STOP = 3;
 const std::chrono::milliseconds BundleThread::KEEP_ALIVE(1000);
 
 BundleThread::BundleThread(CoreBundleContext* ctx)
-  : fwCtx(ctx)
-  , startStopTimeout(0)
+  : startStopTimeout(0)
   , op()
   , be(BundleEvent::BUNDLE_INSTALLED, nullptr)
   , doRun(true)
 {
-  th.v = std::thread(&BundleThread::Run, this);
+  th.v = std::thread(&BundleThread::Run, this, ctx);
 }
 
 BundleThread::~BundleThread()
@@ -63,7 +62,7 @@ void BundleThread::Quit()
   if (th.v.joinable()) th.v.join();
 }
 
-void BundleThread::Run()
+void BundleThread::Run(CoreBundleContext* fwCtx)
 {
   while (doRun)
   {
@@ -90,6 +89,13 @@ void BundleThread::Run()
         break;
       }
 
+      // If we arrive here, we reached our keep-alive limit without
+      // an operation being queued. However, in the mean time, some
+      // other thread may have picked this thread from the bundleThreads.value
+      // list and calling StartAndWait(). So we only terminate this thread
+      // (by returning) if it was not taken from the bundleThreads list to
+      // do more work. Otherwise, we just keep running for another keep alive
+      // cycle.
       {
         auto l2 = fwCtx->bundleThreads.Lock(); US_UNUSED(l2);
         auto iter = std::find(fwCtx->bundleThreads.value.begin(), fwCtx->bundleThreads.value.end(), this->shared_from_this());
@@ -190,7 +196,7 @@ std::exception_ptr BundleThread::StartAndWait(BundlePrivate* b, int operation, U
   bool timeout = false;
   bool uninstall = false;
 
-  fwCtx->resolver.WaitFor(resolveLock, waitTime, [&res]{
+  b->coreCtx->resolver.WaitFor(resolveLock, waitTime, [&res]{
     return res.valid() &&
         res.wait_for(std::chrono::milliseconds::zero()) == US_FUTURE_READY;
   });
@@ -252,23 +258,21 @@ std::exception_ptr BundleThread::StartAndWait(BundlePrivate* b, int operation, U
   }
   else
   {
+    b->coreCtx->bundleThreads.Lock(), b->coreCtx->bundleThreads.value.push_front(this->shared_from_this());
+    if (operation != op.operation)
     {
-      fwCtx->bundleThreads.Lock(), fwCtx->bundleThreads.value.push_front(this->shared_from_this());
-      if (operation != op.operation)
-      {
-        // TODO! Handle when operation has changed.
-        // i.e. uninstall during operation?
-      }
-      b->ResetBundleThread();
-      try
-      {
-        res.get();
-        return nullptr;
-      }
-      catch (...)
-      {
-        return std::current_exception();
-      }
+      // TODO! Handle when operation has changed.
+      // i.e. uninstall during operation?
+    }
+    b->ResetBundleThread();
+    try
+    {
+      res.get();
+      return nullptr;
+    }
+    catch (...)
+    {
+      return std::current_exception();
     }
   }
 }

framework/src/bundle/BundleThread.h

@@ -49,7 +49,6 @@ class BundleThread : public std::enable_shared_from_this<BundleThread>
 
   const static std::chrono::milliseconds KEEP_ALIVE;
 
-  CoreBundleContext* const fwCtx;
   std::chrono::milliseconds startStopTimeout;
 
   struct Op : detail::MultiThreaded<detail::MutexLockingStrategy<>, detail::WaitCondition>
@@ -75,7 +74,7 @@ class BundleThread : public std::enable_shared_from_this<BundleThread>
 
   void Quit();
 
-  void Run();
+  void Run(CoreBundleContext* fwCtx);
 
   void Join();
 

framework/src/bundle/CoreBundleContext.cpp

@@ -169,20 +169,32 @@ void CoreBundleContext::Uninit1()
   listeners.Clear();
   resolver.Clear();
 
+  // Do not hold the bundleThreads lock while calling
+  // BundleThread::Quit() because this will join the bundle
+  // thread which may need to acquire the bundleThreads lock
+  // itself.
+  //
+  // At this point, all bundles are stopped and all
+  // bundle context instances invalidated. So no new bundle
+  // threads can be created and it is sufficient to get
+  // the current list once and not check for new bundle
+  // threads again.
+  std::list<std::shared_ptr<BundleThread>> threads;
+  bundleThreads.Lock(), std::swap(threads, bundleThreads.value);
+
+  while (!threads.empty())
   {
-    auto l = bundleThreads.Lock(); US_UNUSED(l);
-    while (!bundleThreads.value.empty())
-    {
-      bundleThreads.value.front()->Quit();
-      bundleThreads.value.pop_front();
-    }
-    while (!bundleThreads.zombies.empty())
-    {
-      bundleThreads.zombies.front()->Join();
-      bundleThreads.zombies.pop_front();
-    }
+    // Quit the bundle thread. This joins the bundle thread
+    // with this thread and puts it into the zombies list.
+    threads.front()->Quit();
+    threads.pop_front();
   }
 
+  // Clear up all bundle threads that have been quit. At this
+  // point, we do not need to lock the bundleTheads structure
+  // any more.
+  bundleThreads.zombies.clear();
+
   dataStorage.clear();
   storage->Close();
 }

framework/src/util/FrameworkFactory.cpp

@@ -42,7 +42,7 @@ struct CoreBundleContextHolder
     auto const state = ctx->systemBundle->state.load();
     if (((Bundle::STATE_STARTING | Bundle::STATE_ACTIVE) & state) == 0)
     {
-      // Call WaitForStop in case some did call Framework::Stop()
+      // Call WaitForStop in case someone did call Framework::Stop()
       // but didn't wait for it. This joins with a potentially
       // running framework shut down thread.
       ctx->systemBundle->WaitForStop(std::chrono::milliseconds::zero());