From a9b22ad36bebd1a67a313d0727468f2f4fd020e1 Mon Sep 17 00:00:00 2001 From: Vasiliy Date: Thu, 22 Mar 2018 12:45:29 +0300 Subject: [PATCH] issue 348.1: use a view validity lock instead of a view member lock for snapshot readers --- arangod/IResearch/IResearchView.cpp | 59 +++++++++++++++-------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/arangod/IResearch/IResearchView.cpp b/arangod/IResearch/IResearchView.cpp index 6e40e3bd21..b3251b1644 100644 --- a/arangod/IResearch/IResearchView.cpp +++ b/arangod/IResearch/IResearchView.cpp @@ -97,7 +97,7 @@ typedef irs::async_utils::read_write_mutex::write_mutex WriteMutex; //////////////////////////////////////////////////////////////////////////////// class CompoundReader final: public arangodb::iresearch::PrimaryKeyIndexReader { public: - CompoundReader(irs::async_utils::read_write_mutex& mutex); + CompoundReader(ReadMutex& viewMutex); irs::sub_reader const& operator[]( size_t subReaderId ) const noexcept override { @@ -147,15 +147,12 @@ class CompoundReader final: public arangodb::iresearch::PrimaryKeyIndexReader { SubReadersType::const_iterator _itr; }; - // order is important - ReadMutex _mutex; - std::unique_lock _lock; std::vector _readers; SubReadersType _subReaders; + std::lock_guard _viewLock; // prevent data-store deallocation (lock @ AsyncSelf) }; -CompoundReader::CompoundReader(irs::async_utils::read_write_mutex& mutex) - : _mutex(mutex), _lock(_mutex) { +CompoundReader::CompoundReader(ReadMutex& viewMutex): _viewLock(viewMutex) { } void CompoundReader::add(irs::directory_reader const& reader) { @@ -219,7 +216,7 @@ uint64_t CompoundReader::live_docs_count() const { //////////////////////////////////////////////////////////////////////////////// struct ViewState: public arangodb::TransactionState::Cookie { CompoundReader _snapshot; - ViewState(irs::async_utils::read_write_mutex& mutex): _snapshot(mutex) {} + ViewState(ReadMutex& mutex): _snapshot(mutex) {} }; //////////////////////////////////////////////////////////////////////////////// @@ -287,7 +284,7 @@ size_t directoryMemory(irs::directory const& directory, TRI_voc_cid_t viewId) no return size; } -arangodb::iresearch::IResearchLink* findFirstMatchingLink( +std::shared_ptr findFirstMatchingLink( arangodb::LogicalCollection const& collection, arangodb::iresearch::IResearchView const& view ) { @@ -298,7 +295,8 @@ arangodb::iresearch::IResearchLink* findFirstMatchingLink( // TODO FIXME find a better way to retrieve an iResearch Link // cannot use static_cast/reinterpret_cast since Index is not related to IResearchLink - auto* link = dynamic_cast(index.get()); + auto link = + std::dynamic_pointer_cast(index); if (link && *link == view) { return link; // found required link @@ -470,7 +468,7 @@ arangodb::Result updateLinks( struct State { std::shared_ptr _collection; size_t _collectionsToLockOffset; // std::numeric_limits::max() == removal only - arangodb::iresearch::IResearchLink const* _link = nullptr; + std::shared_ptr _link; size_t _linkDefinitionsOffset; bool _valid = true; explicit State(size_t collectionsToLockOffset) @@ -852,17 +850,6 @@ IResearchView::IResearchView( // initialize transaction write callback _trxWriteCallback = [viewPtr](arangodb::TransactionState& state)->void { - /* FIXME TODO recursive read locks may deadlock if a FlushThread tries to - * aquire a write-lock in the middle of a two read locks - * this may occur if a snapshot() is held by the current thread - * in any transaction - */ - if (state.cookie(viewPtr)) { - LOG_TOPIC(WARN, iresearch::IResearchFeature::IRESEARCH) - << "executing collection write operations is not supported with a lock on view '" << viewPtr->name() << "'"; - state.cookie(viewPtr, nullptr); // a temporary workaround for JavaScript tests that lock the view then write to collections - } - switch (state.status()) { case transaction::Status::ABORTED: { auto res = viewPtr->finish(state.id(), false); @@ -1001,7 +988,7 @@ IResearchView::~IResearchView() { { WriteMutex mutex(_asyncSelf->_mutex); SCOPED_LOCK(mutex); // wait for all the view users to finish - _asyncSelf->_value.store(nullptr); // the view is being deallocated, its use is not longer valid + _asyncSelf->_value.store(nullptr); // the view is being deallocated, its use is no longer valid } _asyncTerminate.store(true); // mark long-running async jobs for terminatation @@ -1063,6 +1050,12 @@ void IResearchView::drop() { } } + { + WriteMutex mutex(_asyncSelf->_mutex); + SCOPED_LOCK(mutex); // wait for all the view users to finish + _asyncSelf->_value.store(nullptr); // the view data-stores are being deallocated, view use is no longer valid + } + _asyncTerminate.store(true); // mark long-running async jobs for terminatation { @@ -1092,11 +1085,14 @@ void IResearchView::drop() { try { _storeByTid.clear(); - auto& memoryStore = activeMemoryStore(); - memoryStore._writer->close(); - memoryStore._writer.reset(); - memoryStore._directory->close(); - memoryStore._directory.reset(); + for (size_t i = 0, count = IRESEARCH_COUNTOF(_memoryNodes); i < count; ++i) { + auto& memoryStore = _memoryNodes[i]._store; + + memoryStore._writer->close(); + memoryStore._writer.reset(); + memoryStore._directory->close(); + memoryStore._directory.reset(); + } if (_storePersisted) { _storePersisted._writer->close(); @@ -1824,10 +1820,17 @@ PrimaryKeyIndexReader* IResearchView::snapshot( << "failed to sync while creating snapshot for IResearch view '" << name() << "', previous snapshot will be used instead"; } - auto cookiePtr = irs::memory::make_unique(_mutex); // will aquire read-lock since members can be asynchronously updated + auto cookiePtr = irs::memory::make_unique(_asyncSelf->mutex()); // will aquire read-lock to prevent data-store deallocation auto& reader = cookiePtr->_snapshot; + if (!_asyncSelf->get()) { + return nullptr; // the current view is no longer valid (checked after ReadLock aquisition) + } + try { + ReadMutex mutex(_mutex); // _memoryNodes/_storePersisted can be asynchronously updated + SCOPED_LOCK(mutex); + reader.add(_memoryNode->_store._reader); SCOPED_LOCK(_toFlush->_readMutex); reader.add(_toFlush->_store._reader);