make ownership a bit more transparent, API-wise (#8653)

arangod/MMFiles/MMFilesExportCursor.cpp

@@ -36,15 +36,15 @@
 namespace arangodb {
 
 MMFilesExportCursor::MMFilesExportCursor(TRI_vocbase_t& vocbase, CursorId id,
-                                         arangodb::MMFilesCollectionExport* ex,
+                                         std::unique_ptr<arangodb::MMFilesCollectionExport> ex,
                                          size_t batchSize, double ttl, bool hasCount)
     : Cursor(id, batchSize, ttl, hasCount),
       _guard(vocbase),
-      _ex(ex),
+      _ex(std::move(ex)),
       _position(0),
-      _size(ex->_vpack.size()) {}
+      _size(_ex->_vpack.size()) {}
 
-MMFilesExportCursor::~MMFilesExportCursor() { delete _ex; }
+MMFilesExportCursor::~MMFilesExportCursor() {}
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief check whether the cursor contains more data
@@ -79,6 +79,10 @@ std::pair<aql::ExecutionState, Result> MMFilesExportCursor::dump(VPackBuilder& b
 }
 
 Result MMFilesExportCursor::dumpSync(VPackBuilder& builder) {
+  if (_ex == nullptr) {
+    return Result(TRI_ERROR_CURSOR_NOT_FOUND);
+  }
+
   auto ctx = transaction::StandaloneContext::Create(_guard.database());
   VPackOptions const* oldOptions = builder.options;
 
@@ -135,8 +139,7 @@ Result MMFilesExportCursor::dumpSync(VPackBuilder& builder) {
 
     if (!hasNext()) {
       // mark the cursor as deleted
-      delete _ex;
-      _ex = nullptr;
+      _ex.reset();
       this->setDeleted();
     }
   } catch (arangodb::basics::Exception const& ex) {
@@ -148,7 +151,7 @@ Result MMFilesExportCursor::dumpSync(VPackBuilder& builder) {
                   "internal error during MMFilesExportCursor::dump");
   }
   builder.options = oldOptions;
-  return TRI_ERROR_NO_ERROR;
+  return Result();
 }
 
 std::shared_ptr<transaction::Context> MMFilesExportCursor::context() const {

arangod/MMFiles/MMFilesExportCursor.h

@@ -36,8 +36,8 @@ class MMFilesCollectionExport;
 class MMFilesExportCursor final : public Cursor {
  public:
   MMFilesExportCursor(TRI_vocbase_t& vocbase, CursorId id,
-                      arangodb::MMFilesCollectionExport* ex, size_t batchSize,
-                      double ttl, bool hasCount);
+                      std::unique_ptr<arangodb::MMFilesCollectionExport> ex, 
+                      size_t batchSize, double ttl, bool hasCount);
 
   ~MMFilesExportCursor();
 
@@ -58,7 +58,7 @@ class MMFilesExportCursor final : public Cursor {
 
  private:
   DatabaseGuard _guard;
-  arangodb::MMFilesCollectionExport* _ex;
+  std::unique_ptr<arangodb::MMFilesCollectionExport> _ex;
   size_t _position;
   size_t const _size;
 };

arangod/MMFiles/MMFilesRestExportHandler.cpp

@@ -257,17 +257,12 @@ void MMFilesRestExportHandler::createCursor() {
   auto cursors = _vocbase.cursorRepository();
   TRI_ASSERT(cursors != nullptr);
 
-  Cursor* c = nullptr;
+  auto cursor = std::make_unique<MMFilesExportCursor>(_vocbase, TRI_NewTickServer(),
+                                                      std::move(collectionExport),
+                                                      batchSize, ttl, count);
 
-  {
-    auto cursor = std::make_unique<MMFilesExportCursor>(_vocbase, TRI_NewTickServer(),
-                                                        collectionExport.get(),
-                                                        batchSize, ttl, count);
-
-    collectionExport.release();
-    cursor->use();
-    c = cursors->addCursor(std::move(cursor));
-  }
+  cursor->use();
+  Cursor* c = cursors->addCursor(std::move(cursor));
 
   TRI_ASSERT(c != nullptr);
   TRI_DEFER(cursors->release(c));

arangod/MMFiles/MMFilesRestReplicationHandler.cpp

@@ -669,8 +669,7 @@ void MMFilesRestReplicationHandler::handleCommandCreateKeys() {
   size_t const count = keys->count();
   auto keysRepository = _vocbase.collectionKeys();
 
-  keysRepository->store(keys.get());
-  keys.release();
+  keysRepository->store(std::move(keys));
 
   VPackBuilder result;
   result.add(VPackValue(VPackValueType::Object));

arangod/Utils/CollectionKeysRepository.cpp

@@ -70,11 +70,6 @@ CollectionKeysRepository::~CollectionKeysRepository() {
 
   {
     MUTEX_LOCKER(mutexLocker, _lock);
-
-    for (auto it : _keys) {
-      delete it.second;
-    }
-
     _keys.clear();
   }
 }
@@ -83,9 +78,9 @@ CollectionKeysRepository::~CollectionKeysRepository() {
 /// @brief stores collection keys in the repository
 ////////////////////////////////////////////////////////////////////////////////
 
-void CollectionKeysRepository::store(arangodb::CollectionKeys* keys) {
+void CollectionKeysRepository::store(std::unique_ptr<arangodb::CollectionKeys> keys) {
   MUTEX_LOCKER(mutexLocker, _lock);
-  _keys.emplace(keys->id(), keys);
+  _keys.emplace(keys->id(), std::move(keys));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -93,7 +88,7 @@ void CollectionKeysRepository::store(arangodb::CollectionKeys* keys) {
 ////////////////////////////////////////////////////////////////////////////////
 
 bool CollectionKeysRepository::remove(CollectionKeysId id) {
-  arangodb::CollectionKeys* collectionKeys = nullptr;
+  std::unique_ptr<arangodb::CollectionKeys> owner;
 
   {
     MUTEX_LOCKER(mutexLocker, _lock);
@@ -105,26 +100,28 @@ bool CollectionKeysRepository::remove(CollectionKeysId id) {
       return false;
     }
 
-    collectionKeys = (*it).second;
-
-    if (collectionKeys->isDeleted()) {
+    if ((*it).second->isDeleted()) {
       // already deleted
       return false;
     }
 
-    if (collectionKeys->isUsed()) {
+    if ((*it).second->isUsed()) {
       // keys are in use by someone else. now mark as deleted
-      collectionKeys->deleted();
+      (*it).second->deleted();
       return true;
     }
 
     // keys are not in use by someone else
+    // move ownership for item to our local variable
+    owner = std::move((*it).second);
+    // clear entry from map
     _keys.erase(it);
   }
 
-  TRI_ASSERT(collectionKeys != nullptr);
+  // when we leave this scope, this will fire the dtor of the
+  // entry, outside the lock
+  TRI_ASSERT(owner != nullptr);
 
-  delete collectionKeys;
   return true;
 }
 
@@ -147,14 +144,13 @@ CollectionKeys* CollectionKeysRepository::find(CollectionKeysId id) {
       return nullptr;
     }
 
-    collectionKeys = (*it).second;
-
-    if (collectionKeys->isDeleted()) {
+    if ((*it).second->isDeleted()) {
       // already deleted
       return nullptr;
     }
 
-    collectionKeys->use();
+    (*it).second->use();
+    collectionKeys = (*it).second.get();
   }
 
   return collectionKeys;
@@ -165,6 +161,8 @@ CollectionKeys* CollectionKeysRepository::find(CollectionKeysId id) {
 ////////////////////////////////////////////////////////////////////////////////
 
 void CollectionKeysRepository::release(CollectionKeys* collectionKeys) {
+  std::unique_ptr<CollectionKeys> owner;
+
   {
     MUTEX_LOCKER(mutexLocker, _lock);
 
@@ -175,12 +173,17 @@ void CollectionKeysRepository::release(CollectionKeys* collectionKeys) {
       return;
     }
 
-    // remove from the list
-    _keys.erase(collectionKeys->id());
+    // remove from the list and take ownership
+    auto it = _keys.find(collectionKeys->id());
+
+    if (it != _keys.end()) {
+      owner = std::move((*it).second);
+      _keys.erase(it);
+    }
   }
 
-  // and free the keys
-  delete collectionKeys;
+  // when we get here, the dtor of the entry will be called,
+  // outside the lock
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -190,7 +193,7 @@ void CollectionKeysRepository::release(CollectionKeys* collectionKeys) {
 bool CollectionKeysRepository::containsUsed() {
   MUTEX_LOCKER(mutexLocker, _lock);
 
-  for (auto it : _keys) {
+  for (auto const& it : _keys) {
     if (it.second->isUsed()) {
       return true;
     }
@@ -204,7 +207,7 @@ bool CollectionKeysRepository::containsUsed() {
 ////////////////////////////////////////////////////////////////////////////////
 
 bool CollectionKeysRepository::garbageCollect(bool force) {
-  std::vector<arangodb::CollectionKeys*> found;
+  std::vector<std::unique_ptr<arangodb::CollectionKeys>> found;
   found.reserve(MaxCollectCount);
 
   auto const now = TRI_microtime();
@@ -213,7 +216,7 @@ bool CollectionKeysRepository::garbageCollect(bool force) {
     MUTEX_LOCKER(mutexLocker, _lock);
 
     for (auto it = _keys.begin(); it != _keys.end(); /* no hoisting */) {
-      auto collectionKeys = (*it).second;
+      auto& collectionKeys = (*it).second;
 
       if (collectionKeys->isUsed()) {
         // must not destroy anything currently in use
@@ -227,12 +230,13 @@ bool CollectionKeysRepository::garbageCollect(bool force) {
 
       if (collectionKeys->isDeleted()) {
         try {
-          found.emplace_back(collectionKeys);
-          it = _keys.erase(it);
+          found.emplace_back(std::move(collectionKeys));
         } catch (...) {
           // stop iteration
           break;
         }
+          
+        it = _keys.erase(it);
 
         if (!force && found.size() >= MaxCollectCount) {
           break;
@@ -243,10 +247,7 @@ bool CollectionKeysRepository::garbageCollect(bool force) {
     }
   }
 
-  // remove keys outside the lock
-  for (auto it : found) {
-    delete it;
-  }
-
+  // when we get here, all instances in found will be destroyed
+  // outside the lock
   return (!found.empty());
 }

arangod/Utils/CollectionKeysRepository.h

@@ -52,13 +52,13 @@ class CollectionKeysRepository {
   /// @brief stores collection keys in the repository
   //////////////////////////////////////////////////////////////////////////////
 
-  void store(CollectionKeys*);
+  void store(std::unique_ptr<CollectionKeys>);
 
   //////////////////////////////////////////////////////////////////////////////
-  /// @brief remove a keys by id
+  /// @brief remove a keys entry by id
   //////////////////////////////////////////////////////////////////////////////
 
-  bool remove(CollectionKeysId);
+  bool remove(CollectionKeysId id);
 
   //////////////////////////////////////////////////////////////////////////////
   /// @brief find an existing collection keys list by id
@@ -97,7 +97,7 @@ class CollectionKeysRepository {
   /// @brief list of current keys
   //////////////////////////////////////////////////////////////////////////////
 
-  std::unordered_map<CollectionKeysId, CollectionKeys*> _keys;
+  std::unordered_map<CollectionKeysId, std::unique_ptr<CollectionKeys>> _keys;
 
   //////////////////////////////////////////////////////////////////////////////
   /// @brief maximum number of keys to garbage-collect in one go