From f16b75440c7bb7c00082b44432c9cb79bda6e66a Mon Sep 17 00:00:00 2001 From: Jan Date: Wed, 3 Apr 2019 18:47:04 +0200 Subject: [PATCH] make ownership a bit more transparent, API-wise (#8653) --- arangod/MMFiles/MMFilesExportCursor.cpp | 17 +++-- arangod/MMFiles/MMFilesExportCursor.h | 6 +- arangod/MMFiles/MMFilesRestExportHandler.cpp | 15 ++--- .../MMFiles/MMFilesRestReplicationHandler.cpp | 3 +- arangod/Utils/CollectionKeysRepository.cpp | 67 ++++++++++--------- arangod/Utils/CollectionKeysRepository.h | 8 +-- 6 files changed, 57 insertions(+), 59 deletions(-) diff --git a/arangod/MMFiles/MMFilesExportCursor.cpp b/arangod/MMFiles/MMFilesExportCursor.cpp index 5446df6583..e463732ffd 100644 --- a/arangod/MMFiles/MMFilesExportCursor.cpp +++ b/arangod/MMFiles/MMFilesExportCursor.cpp @@ -36,15 +36,15 @@ namespace arangodb { MMFilesExportCursor::MMFilesExportCursor(TRI_vocbase_t& vocbase, CursorId id, - arangodb::MMFilesCollectionExport* ex, + std::unique_ptr 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 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 MMFilesExportCursor::context() const { diff --git a/arangod/MMFiles/MMFilesExportCursor.h b/arangod/MMFiles/MMFilesExportCursor.h index 2f01b304c5..8829b402ac 100644 --- a/arangod/MMFiles/MMFilesExportCursor.h +++ b/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 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 _ex; size_t _position; size_t const _size; }; diff --git a/arangod/MMFiles/MMFilesRestExportHandler.cpp b/arangod/MMFiles/MMFilesRestExportHandler.cpp index 38fb63ad91..c1d58adf30 100644 --- a/arangod/MMFiles/MMFilesRestExportHandler.cpp +++ b/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(_vocbase, TRI_NewTickServer(), + std::move(collectionExport), + batchSize, ttl, count); - { - auto cursor = std::make_unique(_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)); diff --git a/arangod/MMFiles/MMFilesRestReplicationHandler.cpp b/arangod/MMFiles/MMFilesRestReplicationHandler.cpp index 8875db43b6..b5555b07f9 100644 --- a/arangod/MMFiles/MMFilesRestReplicationHandler.cpp +++ b/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)); diff --git a/arangod/Utils/CollectionKeysRepository.cpp b/arangod/Utils/CollectionKeysRepository.cpp index c9c0238a66..54603058e7 100644 --- a/arangod/Utils/CollectionKeysRepository.cpp +++ b/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 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 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 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 found; + std::vector> 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()); } diff --git a/arangod/Utils/CollectionKeysRepository.h b/arangod/Utils/CollectionKeysRepository.h index ac9a7b983a..44427c5bd4 100644 --- a/arangod/Utils/CollectionKeysRepository.h +++ b/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); ////////////////////////////////////////////////////////////////////////////// - /// @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 _keys; + std::unordered_map> _keys; ////////////////////////////////////////////////////////////////////////////// /// @brief maximum number of keys to garbage-collect in one go