From 75055acca76ae51c4a0d17c81706bcfa4ac2e4f2 Mon Sep 17 00:00:00 2001
From: jsteemann <jsteemann@users.noreply.github.com>
Date: Fri, 17 Sep 2021 00:33:41 +0200
Subject: [PATCH] Avoid acquisition of recursive read lock on server shutdown

* Avoid the acquisition of a recursive read lock on server shutdown, which
  could in theory lead to shutdown hangs at least if a concurrent thread is
  trying to modify the list of collections (very unlikely and never observed
  until now).
---
 CHANGELOG                                   |  5 +++
 arangod/Replication/GlobalInitialSyncer.cpp |  3 +-
 arangod/RestServer/DatabaseFeature.cpp      |  5 ++-
 arangod/VocBase/vocbase.cpp                 | 39 +++++++++++----------
 arangod/VocBase/vocbase.h                   |  5 +--
 5 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index a76a2ec1509e..f40e3428bdb4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,11 @@
 v3.8.2 (XXXX-XX-XX)
 -------------------
 
+* Avoid the acquisition of a recursive read lock on server shutdown, which
+  could in theory lead to shutdown hangs at least if a concurrent thread is
+  trying to modify the list of collections (very unlikely and never observed
+  until now).
+
 * Fix active failover, so that the new host actually has working
   foxx services. (BTS-558)
 
diff --git a/arangod/Replication/GlobalInitialSyncer.cpp b/arangod/Replication/GlobalInitialSyncer.cpp
index 35c43c8e3f56..26db31ebffde 100644
--- a/arangod/Replication/GlobalInitialSyncer.cpp
+++ b/arangod/Replication/GlobalInitialSyncer.cpp
@@ -303,8 +303,7 @@ Result GlobalInitialSyncer::updateServerInventory(VPackSlice const& leaderDataba
             if (!collection->system()) {  // we will not drop system collections here
               toDrop.emplace_back(collection);
             }
-          },
-          false);
+          });
 
       for (auto const& collection : toDrop) {
         try {
diff --git a/arangod/RestServer/DatabaseFeature.cpp b/arangod/RestServer/DatabaseFeature.cpp
index 105b018c5374..b830b208137a 100644
--- a/arangod/RestServer/DatabaseFeature.cpp
+++ b/arangod/RestServer/DatabaseFeature.cpp
@@ -487,14 +487,13 @@ void DatabaseFeature::stop() {
 #endif
     vocbase->stop();
 
-    vocbase->processCollections(
+    vocbase->processCollectionsOnShutdown(
         [](LogicalCollection* collection) {
           // no one else must modify the collection's status while we are in
           // here
           collection->executeWhileStatusWriteLocked(
               [collection]() { collection->close(); });
-        },
-        true);
+        });
 
 #ifdef ARANGODB_ENABLE_MAINTAINER_MODE
     // i am here for debugging only.
diff --git a/arangod/VocBase/vocbase.cpp b/arangod/VocBase/vocbase.cpp
index a4e2e3da01d5..0049d1396add 100644
--- a/arangod/VocBase/vocbase.cpp
+++ b/arangod/VocBase/vocbase.cpp
@@ -1736,33 +1736,34 @@ std::vector<std::shared_ptr<arangodb::LogicalView>> TRI_vocbase_t::views() {
   return views;
 }
 
-void TRI_vocbase_t::processCollections(std::function<void(LogicalCollection*)> const& cb,
-                                       bool includeDeleted) {
+void TRI_vocbase_t::processCollectionsOnShutdown(std::function<void(LogicalCollection*)> const& cb) {
+  RECURSIVE_WRITE_LOCKER(_dataSourceLock, _dataSourceLockWriteOwner);
+
+  for (auto const& it : _collections) {
+    cb(it.get());
+  }
+}
+
+void TRI_vocbase_t::processCollections(std::function<void(LogicalCollection*)> const& cb) {
   RECURSIVE_READ_LOCKER(_dataSourceLock, _dataSourceLockWriteOwner);
 
-  if (includeDeleted) {
-    for (auto const& it : _collections) {
-      cb(it.get());
-    }
-  } else {
-    for (auto& entry : _dataSourceById) {
-      TRI_ASSERT(entry.second);
+  for (auto& entry : _dataSourceById) {
+    TRI_ASSERT(entry.second);
 
-      if (entry.second->category() != LogicalCollection::category()) {
-        continue;
-      }
+    if (entry.second->category() != LogicalCollection::category()) {
+      continue;
+    }
 
 #ifdef ARANGODB_ENABLE_MAINTAINER_MODE
-      auto collection =
-          std::dynamic_pointer_cast<arangodb::LogicalCollection>(entry.second);
-      TRI_ASSERT(collection);
+    auto collection =
+        std::dynamic_pointer_cast<arangodb::LogicalCollection>(entry.second);
+    TRI_ASSERT(collection);
 #else
-      auto collection =
-          std::static_pointer_cast<arangodb::LogicalCollection>(entry.second);
+    auto collection =
+        std::static_pointer_cast<arangodb::LogicalCollection>(entry.second);
 #endif
 
-      cb(collection.get());
-    }
+    cb(collection.get());
   }
 }
 
diff --git a/arangod/VocBase/vocbase.h b/arangod/VocBase/vocbase.h
index 90cfe1bf87fc..b912104fad7f 100644
--- a/arangod/VocBase/vocbase.h
+++ b/arangod/VocBase/vocbase.h
@@ -252,8 +252,9 @@ struct TRI_vocbase_t {
   /// @brief returns all known collections
   std::vector<std::shared_ptr<arangodb::LogicalCollection>> collections(bool includeDeleted);
 
-  void processCollections(std::function<void(arangodb::LogicalCollection*)> const& cb,
-                          bool includeDeleted);
+  void processCollectionsOnShutdown(std::function<void(arangodb::LogicalCollection*)> const& cb);
+
+  void processCollections(std::function<void(arangodb::LogicalCollection*)> const& cb);
 
   /// @brief returns names of all known collections
   std::vector<std::string> collectionNames();