8000 issue 485: ensure LogicalDataSource::drop() is called on vocbase drop… · mnemosdev/arangodb@18b2eb6 · GitHub
[go: up one dir, main page]
Skip to content

Commit 18b2eb6

Browse files
vasiliy-arangodbAndrey Abramov
vasiliy-arangodb
authored and
Andrey Abramov
committed
issue 485: ensure LogicalDataSource::drop() is called on vocbase drop (arangodb#6895)
* issue 485: ensure LogicalDataSource::drop() is called on vocbase drop * add missed change * backport: address race between make(...) and async job * add another missed change * backport: ensure recursive lock reports itself as locked correctly * backport: address test failure on mmfiles * backport: remove redundant lock already held by async task * backport: reset reader before unlinking directory
1 parent edaa127 commit 18b2eb6

File tree

9 files changed

+217
-57
lines changed

9 files changed

+217
-57
lines changed

‎arangod/IResearch/IResearchView.cpp

Lines changed: 44 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -792,9 +792,6 @@ IResearchView::IResearchView(
792792
auto const runCleanupAfterConsolidation =
793793
state._cleanupIntervalCount > state._cleanupIntervalStep;
794794

795-
auto& viewMutex = self()->mutex();
796-
SCOPED_LOCK(viewMutex); // ensure view does not get deallocated before call back finishes
797-
798795
if (_storePersisted
799796
&& consolidateCleanupStore(
800797
*_storePersisted._directory,
@@ -1092,6 +1089,7 @@ arangodb::Result IResearchView::drop(
10921089
}
10931090

10941091
arangodb::Result IResearchView::dropImpl() {
1092+
std::unordered_set<TRI_voc_cid_t> collections;
10951093
std::unordered_set<TRI_voc_cid_t> stale;
10961094

10971095
// drop all known links
@@ -1101,41 +1099,42 @@ arangodb::Result IResearchView::dropImpl() {
11011099
stale = _metaState._collections;
11021100
}
11031101

1104-
// check link auth as per https://github.com/arangodb/backlog/issues/459
1105-
if (arangodb::ExecContext::CURRENT) {
1106-
for (auto& entry: stale) {
1107-
auto collection = vocbase().lookupCollection(entry);
1102+
if (!stale.empty()) {
1103+
// check link auth as per https://github.com/arangodb/backlog/issues/459
1104+
if (arangodb::ExecContext::CURRENT) {
1105+
for (auto& entry: stale) {
1106+
auto collection = vocbase().lookupCollection(entry);
11081107

1109-
if (collection
1110-
&& !arangodb::ExecContext::CURRENT->canUseCollection(vocbase().name(), collection->name(), arangodb::auth::Level::RO)) {
1111-
return arangodb::Result(TRI_ERROR_FORBIDDEN);
1108+
if (collection
1109+
&& !arangodb::ExecContext::CURRENT->canUseCollection(vocbase().name(), collection->name(), arangodb::auth::Level::RO)) {
1110+
return arangodb::Result(TRI_ERROR_FORBIDDEN);
1111+
}
11121112
}
11131113
}
1114-
}
11151114

1116-
std::unordered_set<TRI_voc_cid_t> collections;
1117-
arangodb::Result res;
1115+
arangodb::Result res;
11181116

1119-
{
1120-
if (!_updateLinksLock.try_lock()) {
1121-
return arangodb::Result(
1122-
TRI_ERROR_FAILED, // FIXME use specific error code
1123-
std::string("failed to remove arangosearch view '") + name()
1124-
);
1125-
}
1117+
{
1118+
if (!_updateLinksLock.try_lock()) {
1119+
return arangodb::Result(
1120+
TRI_ERROR_FAILED, // FIXME use specific error code
1121+
std::string("failed to remove arangosearch view '") + name()
1122+
);
1123+
}
11261124

1127-
ADOPT_SCOPED_LOCK_NAMED(_updateLinksLock, lock);
1125+
ADOPT_SCOPED_LOCK_NAMED(_updateLinksLock, lock);
11281126

1129-
res = IResearchLinkHelper::updateLinks(
1130-
collections, vocbase(), *this, emptyObjectSlice(), stale
1131-
);
1132-
}
1127+
res = IResearchLinkHelper::updateLinks(
1128+
collections, vocbase(), *this, emptyObjectSlice(), stale
1129+
);
1130+
}
11331131

1134-
if (!res.ok()) {
1135-
return arangodb::Result(
1136-
res.errorNumber(),
1137-
std::string("failed to remove links while removing arangosearch view '") + name() + "': " + res.errorMessage()
1138-
);
1132+
if (!res.ok()) {
1133+
return arangodb::Result(
1134+
res.errorNumber(),
1135+
std::string("failed to remove links while removing arangosearch view '") + name() + "': " + res.errorMessage()
1136+
);
1137+
}
11391138
}
11401139

11411140
_asyncTerminate.store(true); // mark long-running async jobs for terminatation
@@ -1173,6 +1172,7 @@ arangodb::Result IResearchView::dropImpl() {
11731172
// ...........................................................................
11741173
try {
11751174
if (_storePersisted) {
1175+
_storePersisted._reader.reset(); // reset reader to release file handles
11761176
_storePersisted._writer->close();
11771177
_storePersisted._writer.reset();
11781178
_storePersisted._directory->close();
@@ -1531,13 +1531,20 @@ int IResearchView::insert(
15311531
auto& properties = info.isObject() ? info : emptyObjectSlice(); // if no 'info' then assume defaults
15321532
std::string error;
15331533

1534-
if (!impl._meta->init(properties, error)
1535-
|| !impl._metaState.init(properties, error)) {
1536-
TRI_set_errno(TRI_ERROR_BAD_PARAMETER);
1537-
LOG_TOPIC(WARN, arangodb::iresearch::TOPIC)
1538-
<< "failed to initialize arangosearch view from definition, error: " << error;
1534+
{
1535+
WriteMutex mutex(impl._mutex); // '_meta' can be asynchronously read by async jobs started in constructor
1536+
SCOPED_LOCK(mutex);
15391537

1540-
return nullptr;
1538+
if (!impl._meta->init(properties, error)
1539+
|| !impl._metaState.init(properties, error)) {
1540+
TRI_set_errno(TRI_ERROR_BAD_PARAMETER);
1541+
LOG_TOPIC(WARN, arangodb::iresearch::TOPIC)
1542+
<< "failed to initialize arangosearch view from definition, error: " << error;
1543+
1544+
return nullptr;
1545+
}
1546+
1547+
impl.updateProperties(impl._meta); // trigger reload of settings for async jobs
15411548
}
15421549

15431550
auto links = properties.hasKey(StaticStrings::LinksField)
@@ -1629,6 +1636,7 @@ size_t IResearchView::memory() const {
16291636
size += _metaState.memory();
16301637

16311638
if (_storePersisted) {
1639+
// FIXME TODO this is incorrect since '_storePersisted' is on disk and not in memory
16321640
size += directoryMemory(*(_storePersisted._directory), id());
16331641
size += _storePersisted._path.native().size() * sizeof(irs::utf8_path::native_char_t);
16341642
}

‎arangod/IResearch/IResearchView.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,6 @@ class IResearchView final
318318
irs::directory::ptr _directory;
319319
irs::directory_reader _reader;
320320
irs::index_reader::ptr _readerImpl; // need this for 'std::atomic_exchange_strong'
321-
std::atomic<size_t> _segmentCount{}; // FIXME remove total number of segments in the writer
322321
irs::index_writer::ptr _writer;
323322

324323
DataStore() = default;

‎arangod/RestServer/DatabaseFeature.cpp

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -638,7 +638,7 @@ int DatabaseFeature::dropDatabase(std::string const& name, bool waitForDeletion,
638638

639639
StorageEngine* engine = EngineSelectorFeature::ENGINE;
640640
TRI_voc_tick_t id = 0;
641-
int res;
641+
int res = TRI_ERROR_NO_ERROR;
642642
{
643643
MUTEX_LOCKER(mutexLocker, _databasesMutex);
644644

@@ -649,19 +649,48 @@ int DatabaseFeature::dropDatabase(std::string const& name, bool waitForDeletion,
649649
newLists = new DatabasesLists(*oldLists);
650650

651651
auto it = newLists->_databases.find(name);
652+
652653
if (it == newLists->_databases.end()) {
653654
// not found
654655
delete newLists;
655656
events::DropDatabase(name, TRI_ERROR_ARANGO_DATABASE_NOT_FOUND);
656657
return TRI_ERROR_ARANGO_DATABASE_NOT_FOUND;
657-
} else {
658-
vocbase = it->second;
659-
id = vocbase->id();
660-
// mark as deleted
658+
}
659+
660+
vocbase = it->second;
661+
id = vocbase->id();
662+
// mark as deleted
663+
664+
// call LogicalDataSource::drop() to allow instances to clean up internal
665+
// state (e.g. for LogicalView implementations)
666+
TRI_vocbase_t::dataSourceVisitor visitor = [&res, &vocbase](
667+
arangodb::LogicalDataSource& dataSource
668+
)->bool {
669+
// skip LogicalCollection since their internal state is always in the
670+
// StorageEngine (optimization)
671+
if (arangodb::LogicalCollection::category() == dataSource.category()) {
672+
return true;
673+
}
661674

662-
newLists->_databases.erase(it);
663-
newLists->_droppedDatabases.insert(vocbase);
675+
auto result = dataSource.drop();
676+
677+
if (!result.ok()) {
678+
res = result.errorNumber();
679+
LOG_TOPIC(FATAL, arangodb::Logger::FIXME)
680+
<< "failed to drop DataSource '" << dataSource.name() << "' while dropping database '" << vocbase->name() << "': " << result.errorNumber() << " " << result.errorMessage();
681+
}
682+
683+
return true; // try next DataSource
684+
};
685+
686+
vocbase->visitDataSources(visitor, true); // aquire a write lock to avoid potential deadlocks
687+
688+
if (TRI_ERROR_NO_ERROR != res) {
689+
return res;
664690
}
691+
692+
newLists->_databases.erase(it);
693+
newLists->_droppedDatabases.insert(vocbase);
665694
} catch (...) {
666695
delete newLists;
667696
return TRI_ERROR_OUT_OF_MEMORY;

‎arangod/VocBase/LogicalView.cpp

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -289,12 +289,17 @@ arangodb::Result LogicalViewStorageEngine::appendVelocyPack(
289289
}
290290

291291
arangodb::Result LogicalViewStorageEngine::drop() {
292+
if (deleted()) {
293+
return Result(); // view already dropped
294+
}
295+
292296
TRI_ASSERT(!ServerState::instance()->isCoordinator());
293297
StorageEngine* engine = EngineSelectorFeature::ENGINE;
294298
TRI_ASSERT(engine);
295299
auto res = dropImpl();
296300

297-
if (res.ok()) {
301+
// skip on error or if already called by dropImpl()
302+
if (res.ok() && !deleted()) {
298303
deleted(true);
299304
engine->dropView(vocbase(), *this);
300305
}
@@ -373,4 +378,4 @@ arangodb::Result LogicalViewStorageEngine::updateProperties(
373378

374379
// -----------------------------------------------------------------------------
375380
// --SECTION-- END-OF-FILE
376-
// -----------------------------------------------------------------------------
381+
// -----------------------------------------------------------------------------

‎arangod/VocBase/vocbase.cpp

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,10 @@ namespace {
9898
bool acquire,
9999
char const* file,
100100
int line
101-
): _locker(&mutex, type, false, file, line), _owner(owner), _update(noop) {
101+
): _locked(false),
102+
_locker(&mutex, type, false, file, line),
103+
_owner(owner),
104+
_update(noop) {
102105
if (acquire) {
103106
lock();
104107
}
@@ -108,9 +111,7 @@ namespace {
108111
unlock();
109112
}
110113

111-
bool isLocked() {
112-
return _locker.isLocked();
113-
}
114+
bool isLocked() { return _locked; }
114115

115116
void lock() {
116117
// recursive locking of the same instance is not yet supported (create a new instance instead)
@@ -121,13 +122,17 @@ namespace {
121122
_owner.store(std::this_thread::get_id());
122123
_update = owned;
123124
}
125+
126+
_locked = true;
124127
}
125128

126 10000 129
void unlock() {
127130
_update(*this);
131+
_locked = false;
128132
}
129133

130134
private:
135+
bool _locked; // track locked state separately for recursive lock aquisition
131136
arangodb::basics::WriteLocker<T> _locker;
132137
std::atomic<std::thread::id>& _owner;
133138
void (*_update)(RecursiveWriteLocker& locker);
@@ -1859,6 +1864,10 @@ TRI_vocbase_t::~TRI_vocbase_t() {
18591864
for (auto& it : _collections) {
18601865
it->close(); // required to release indexes
18611866
}
1867+
1868+
_dataSourceById.clear(); // clear map before deallocating TRI_vocbase_t members
1869+
_dataSourceByName.clear(); // clear map before deallocating TRI_vocbase_t members
1870+
_dataSourceByUuid.clear(); // clear map before deallocating TRI_vocbase_t members
18621871
}
18631872

18641873
std::string TRI_vocbase_t::path() const {
@@ -2123,6 +2132,43 @@ std::vector<std::shared_ptr<arangodb::LogicalCollection>> TRI_vocbase_t::collect
21232132
return collections;
21242133
}
21252134

2135+
bool TRI_vocbase_t::visitDataSources(
2136+
dataSourceVisitor const& visitor,
2137+
bool lockWrite /*= false*/
2138+
) {
2139+
TRI_ASSERT(visitor);
2140+
2141+
if (!lockWrite) {
2142+
RECURSIVE_READ_LOCKER(_dataSourceLock, _dataSourceLockWriteOwner);
2143+
2144+
for (auto& entry: _dataSourceById) {
2145+
if (entry.second && !visitor(*(entry.second))) {
2146+
return false;
2147+
}
2148+
}
2149+
2150+
return true;
2151+
}
2152+
2153+
RECURSIVE_WRITE_LOCKER(_dataSourceLock, _dataSourceLockWriteOwner);
2154+
std::vector<std::shared_ptr<arangodb::LogicalDataSource>> dataSources;
2155+
2156+
dataSources.reserve(_dataSourceById.size());
2157+
2158+
// create a copy of all the datasource in case 'visitor' modifies '_dataSourceById'
2159+
for (auto& entry: _dataSourceById) {
2160+
dataSources.emplace_back(entry.second);
2161+
}
2162+
2163+
for (auto& dataSource: dataSources) {
2164+
if (dataSource && !visitor(*dataSource)) {
2165+
return false;
2166+
}
2167+
}
2168+
2169+
return true;
2170+
}
2171+
21262172
/// @brief extract the _rev attribute from a slice
21272173
TRI_voc_rid_t TRI_ExtractRevisionId(VPackSlice slice) {
21282174
slice = slice.resolveExternal();
@@ -2248,4 +2294,4 @@ TRI_voc_rid_t TRI_StringToRid(char const* p, size_t len, bool& isOld,
22482294

22492295
// -----------------------------------------------------------------------------
22502296
// --SECTION-- END-OF-FILE
2251-
// -----------------------------------------------------------------------------
2297+
// -----------------------------------------------------------------------------

‎arangod/VocBase/vocbase.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,16 @@ struct TRI_vocbase_t {
390390
/// @brief releases a collection from usage
391391
void releaseCollection(arangodb::LogicalCollection* collection);
392392

393+
/// @brief visit all DataSources registered with this vocbase
394+
/// @param visitor returns if visitation should continue
395+
/// @param lockWrite aquire write lock (if 'visitor' will modify vocbase)
396+
/// @return visitation compleated successfully
397+
typedef std::function<bool(arangodb::LogicalDataSource& dataSource)> dataSourceVisitor;
398+
bool visitDataSources(
399+
dataSourceVisitor const& visitor,
400+
bool lockWrite = false
401+
);
402+
393403
priv 42BD ate:
394404

395405
/// @brief check some invariants on the various lists of collections

0 commit comments

Comments
 (0)
0