From 18e8fd25c6e6555754d8c9841c6588a64272f847 Mon Sep 17 00:00:00 2001 From: Ben Langmuir Date: Mon, 11 May 2020 16:22:03 -0700 Subject: [PATCH] [tsan] Fix a race during processUnitsAndWait() After we added processUnitsAndWait() as a separte entry point that ultimately called the onFilesChange() method, it created a race on the DoneInitState, which was accessed directly in the unit event callback before jumping to the unit processing queue. Simplify that logic and fix the race by using processUnitsAndWait() during initialization if we desire waiting, and get rid of DoneInitState. The previous approach was more flexible about allowing you to wait at an arbitrary point after creating the IndexDatastore, but in practice we were not using that ability, and it made it harder to understand the synchronization required. --- include/IndexStoreDB/Index/IndexSystem.h | 3 +- lib/CIndexStoreDB/CIndexStoreDB.cpp | 5 +- lib/Index/IndexDatastore.cpp | 84 ++++-------------------- lib/Index/IndexDatastore.h | 3 +- lib/Index/IndexSystem.cpp | 16 ++--- 5 files changed, 21 insertions(+), 90 deletions(-) diff --git a/include/IndexStoreDB/Index/IndexSystem.h b/include/IndexStoreDB/Index/IndexSystem.h index 85c9852a..8610c3f9 100644 --- a/include/IndexStoreDB/Index/IndexSystem.h +++ b/include/IndexStoreDB/Index/IndexSystem.h @@ -50,11 +50,10 @@ class INDEXSTOREDB_EXPORT IndexSystem { bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, Optional initialDBSize, std::string &Error); - void waitUntilDoneInitializing(); - bool isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles); bool isUnitOutOfDate(StringRef unitOutputPath, llvm::sys::TimePoint<> outOfDateModTime); diff --git a/lib/CIndexStoreDB/CIndexStoreDB.cpp b/lib/CIndexStoreDB/CIndexStoreDB.cpp index 059bf029..0d7da811 100644 --- a/lib/CIndexStoreDB/CIndexStoreDB.cpp +++ b/lib/CIndexStoreDB/CIndexStoreDB.cpp @@ -114,12 +114,9 @@ indexstoredb_index_create(const char *storePath, const char *databasePath, if (auto index = IndexSystem::create(storePath, databasePath, libProviderObj, delegate, useExplicitOutputUnits, readonly, - /*enableOutOfDateFileWatching=*/false, listenToUnitEvents, + /*enableOutOfDateFileWatching=*/false, listenToUnitEvents, wait, llvm::None, errMsg)) { - if (wait) - index->waitUntilDoneInitializing(); - return make_object(index); } else if (error) { diff --git a/lib/Index/IndexDatastore.cpp b/lib/Index/IndexDatastore.cpp index a00f4cba..de5be6c3 100644 --- a/lib/Index/IndexDatastore.cpp +++ b/lib/Index/IndexDatastore.cpp @@ -88,11 +88,6 @@ struct UnitEventInfo { : kind(kind), name(std::move(name)), isDependency(isDependency) {} }; -struct DoneInitState { - std::atomic DoneInit{false}; - unsigned RemainingInitUnits = 0; // this is not accessed concurrently. -}; - struct PollUnitsState { llvm::sys::Mutex pollMtx; llvm::StringMap> knownUnits; @@ -108,9 +103,6 @@ class StoreUnitRepo : public std::enable_shared_from_this { std::shared_ptr PathWatcher; - DoneInitState InitializingState; - dispatch_semaphore_t InitSemaphore; - PollUnitsState pollUnitsState; mutable llvm::sys::Mutex StateMtx; @@ -129,10 +121,6 @@ class StoreUnitRepo : public std::enable_shared_from_this { EnableOutOfDateFileWatching(enableOutOfDateFileWatching), Delegate(std::move(Delegate)), CanonPathCache(std::move(canonPathCache)) { - InitSemaphore = dispatch_semaphore_create(0); - } - ~StoreUnitRepo() { - dispatch_release(InitSemaphore); } void onFilesChange(std::vector evts, @@ -140,11 +128,6 @@ class StoreUnitRepo : public std::enable_shared_from_this { function_ref ReportCompleted, function_ref DirectoryDeleted); - void setInitialUnitCount(unsigned count); - void processedInitialUnitCount(unsigned count); - void finishedUnitInitialization(); - void waitUntilDoneInitializing(); - /// *For Testing* Poll for any changes to units and wait until they have been registered. void pollForUnitChangesAndWait(); @@ -185,9 +168,9 @@ class IndexDatastoreImpl { bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, std::string &Error); - void waitUntilDoneInitializing(); bool isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles); bool isUnitOutOfDate(StringRef unitOutputPath, llvm::sys::TimePoint<> outOfDateModTime); void checkUnitContainingFileIsOutOfDate(StringRef file); @@ -464,10 +447,6 @@ void StoreUnitRepo::onFilesChange(std::vector evts, }; PathWatcher = std::make_shared(std::move(pathEventsReceiver)); } - - if (!InitializingState.DoneInit) { - processedInitialUnitCount(evts.size()); - } } void StoreUnitRepo::registerUnit(StringRef unitName, std::shared_ptr processSession) { @@ -761,30 +740,6 @@ void StoreUnitRepo::purgeStaleData() { // IdxStore->purgeStaleRecords(ActiveRecNames); } -void StoreUnitRepo::setInitialUnitCount(unsigned count) { - InitializingState.RemainingInitUnits = count; -} - -void StoreUnitRepo::processedInitialUnitCount(unsigned count) { - assert(!InitializingState.DoneInit); - InitializingState.RemainingInitUnits -= std::min(count, InitializingState.RemainingInitUnits); - if (InitializingState.RemainingInitUnits == 0) { - finishedUnitInitialization(); - } -} - -void StoreUnitRepo::finishedUnitInitialization() { - assert(!InitializingState.DoneInit); - dispatch_semaphore_signal(InitSemaphore); - InitializingState.DoneInit = true; -} - -void StoreUnitRepo::waitUntilDoneInitializing() { - if (InitializingState.DoneInit) - return; - dispatch_semaphore_wait(InitSemaphore, DISPATCH_TIME_FOREVER); -} - void StoreUnitRepo::pollForUnitChangesAndWait() { sys::ScopedLock L(pollUnitsState.pollMtx); std::vector events; @@ -1081,6 +1036,7 @@ bool IndexDatastoreImpl::init(IndexStoreRef idxStore, bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, std::string &Error) { this->IdxStore = std::move(idxStore); if (!this->IdxStore) @@ -1092,18 +1048,8 @@ bool IndexDatastoreImpl::init(IndexStoreRef idxStore, auto UnitRepo = std::make_shared(this->IdxStore, SymIndex, useExplicitOutputUnits, enableOutOfDateFileWatching, Delegate, CanonPathCache); std::weak_ptr WeakUnitRepo = UnitRepo; auto eventsDeque = std::make_shared(); - auto OnUnitsChange = [WeakUnitRepo, Delegate, eventsDeque](IndexStore::UnitEventNotification EventNote) { - if (EventNote.isInitial()) { - auto UnitRepo = WeakUnitRepo.lock(); - if (!UnitRepo) - return; - size_t evtCount = EventNote.getEventsCount(); - if (evtCount == 0) { - UnitRepo->finishedUnitInitialization(); - } else { - UnitRepo->setInitialUnitCount(evtCount); - } - } + auto OnUnitsChange = [WeakUnitRepo, Delegate, eventsDeque, waitUntilDoneInitializing](IndexStore::UnitEventNotification EventNote) { + bool shouldWait = waitUntilDoneInitializing && EventNote.isInitial(); std::vector evts; for (size_t i = 0, e = EventNote.getEventsCount(); i != e; ++i) { @@ -1112,25 +1058,23 @@ bool IndexDatastoreImpl::init(IndexStoreRef idxStore, } auto session = std::make_shared(eventsDeque, WeakUnitRepo, Delegate); - session->process(std::move(evts), /*waitForProcessing=*/false); + session->process(std::move(evts), shouldWait); }; + this->UnitRepo = std::move(UnitRepo); + if (listenToUnitEvents) { this->IdxStore->setUnitEventHandler(OnUnitsChange); - bool err = this->IdxStore->startEventListening(/*waitInitialSync=*/false, Error); + bool err = this->IdxStore->startEventListening(waitUntilDoneInitializing, Error); if (err) return true; + } else if (waitUntilDoneInitializing) { + pollForUnitChangesAndWait(); } - this->UnitRepo = std::move(UnitRepo); return false; } -void IndexDatastoreImpl::waitUntilDoneInitializing() { - if (UnitRepo) - UnitRepo->waitUntilDoneInitializing(); -} - bool IndexDatastoreImpl::isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles) { auto mostRecentFileAndTime = UnitMonitor::getMostRecentModTime(dirtyFiles); return isUnitOutOfDate(unitOutputPath, mostRecentFileAndTime.second); @@ -1182,10 +1126,12 @@ IndexDatastore::create(IndexStoreRef idxStore, bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, std::string &Error) { std::unique_ptr Impl(new IndexDatastoreImpl()); bool Err = Impl->init(std::move(idxStore), std::move(SymIndex), std::move(Delegate), std::move(CanonPathCache), - useExplicitOutputUnits, readonly, enableOutOfDateFileWatching, listenToUnitEvents, Error); + useExplicitOutputUnits, readonly, enableOutOfDateFileWatching, + listenToUnitEvents, waitUntilDoneInitializing, Error); if (Err) return nullptr; @@ -1200,10 +1146,6 @@ IndexDatastore::~IndexDatastore() { delete IMPL; } -void IndexDatastore::waitUntilDoneInitializing() { - return IMPL->waitUntilDoneInitializing(); -} - bool IndexDatastore::isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles) { return IMPL->isUnitOutOfDate(unitOutputPath, dirtyFiles); } diff --git a/lib/Index/IndexDatastore.h b/lib/Index/IndexDatastore.h index edd77601..db5e9906 100644 --- a/lib/Index/IndexDatastore.h +++ b/lib/Index/IndexDatastore.h @@ -44,10 +44,9 @@ class IndexDatastore { bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, std::string &Error); - void waitUntilDoneInitializing(); - bool isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles); bool isUnitOutOfDate(StringRef unitOutputPath, llvm::sys::TimePoint<> outOfDateModTime); diff --git a/lib/Index/IndexSystem.cpp b/lib/Index/IndexSystem.cpp index ff737eaa..c5f506e4 100644 --- a/lib/Index/IndexSystem.cpp +++ b/lib/Index/IndexSystem.cpp @@ -119,11 +119,10 @@ class IndexSystemImpl { std::shared_ptr Delegate, bool useExplicitOutputUnits, bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, Optional initialDBSize, std::string &Error); - void waitUntilDoneInitializing(); - bool isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles); bool isUnitOutOfDate(StringRef unitOutputPath, llvm::sys::TimePoint<> outOfDateModTime); void checkUnitContainingFileIsOutOfDate(StringRef file); @@ -210,6 +209,7 @@ bool IndexSystemImpl::init(StringRef StorePath, std::shared_ptr Delegate, bool useExplicitOutputUnits, bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, Optional initialDBSize, std::string &Error) { this->StorePath = StorePath; @@ -252,6 +252,7 @@ bool IndexSystemImpl::init(StringRef StorePath, readonly, enableOutOfDateFileWatching, listenToUnitEvents, + waitUntilDoneInitializing, Error); if (!this->IndexStore) @@ -259,10 +260,6 @@ bool IndexSystemImpl::init(StringRef StorePath, return false; } -void IndexSystemImpl::waitUntilDoneInitializing() { - IndexStore->waitUntilDoneInitializing(); -} - bool IndexSystemImpl::isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles) { return IndexStore->isUnitOutOfDate(unitOutputPath, dirtyFiles); } @@ -611,12 +608,13 @@ IndexSystem::create(StringRef StorePath, std::shared_ptr Delegate, bool useExplicitOutputUnits, bool readonly, bool enableOutOfDateFileWatching, bool listenToUnitEvents, + bool waitUntilDoneInitializing, Optional initialDBSize, std::string &Error) { std::unique_ptr Impl(new IndexSystemImpl()); bool Err = Impl->init(StorePath, dbasePath, std::move(storeLibProvider), std::move(Delegate), useExplicitOutputUnits, readonly, - enableOutOfDateFileWatching, listenToUnitEvents, + enableOutOfDateFileWatching, listenToUnitEvents, waitUntilDoneInitializing, initialDBSize, Error); if (Err) return nullptr; @@ -632,10 +630,6 @@ IndexSystem::~IndexSystem() { delete IMPL; } -void IndexSystem::waitUntilDoneInitializing() { - return IMPL->waitUntilDoneInitializing(); -} - bool IndexSystem::isUnitOutOfDate(StringRef unitOutputPath, ArrayRef dirtyFiles) { return IMPL->isUnitOutOfDate(unitOutputPath, dirtyFiles); }