From fe7b5458eba417fee38e8cb8ea8060554df7c091 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Fri, 5 Mar 2021 13:54:50 +0100 Subject: [PATCH] Refactor tst_joinAllThreads, to be used in ModelManager d'tor The idea in this approach is that we only collect those futures, which have resulted from runAsync. The assumption is that all tasks associated with those futures may sooner or later finish, without the need to call qApp->processEvents(). OTOH, we don't collect fake futures coming from Utils::onFinished, as these requires the spinning event loop in order to deliver the onFinished signal. So, the new joinAllThreads() method waits for all collected futures to finish. We also _do_ want canceled and not finished futures to finish, since even when they are canceled, they may still be running and using the internals of possibly destructed ModelManager. This means, we are only waiting for other threads to be finished, without reporting their results to e.g. onFinished() handlers. Some tests require that all onFinished handlers are also processed. In order to achieve this, we create a loop inside tst_joinAllThreads() method and we call joinAllThreads(), so it will wait for all pending queue to finish, and then we call process events, in order to let finished futures propagate their results to their respective onFinished() handlers. Some handlers may have stared another threads when being processed, so we may expect that some new futures will appear. So, after processing the events we check if any new events appeared, and in this case we repeat the loop. Otherwise, we finish synchronization. Amends: 96c860159b862460e21be16a6e2839c0b591e016 Task-number: QTCREATORBUG-25350 Change-Id: I5e44150c55f6be00445a5695938482d948990c94 Reviewed-by: Fawzi Mohamed --- src/libs/qmljs/qmljsmodelmanagerinterface.cpp | 71 +++++++------------ src/libs/qmljs/qmljsmodelmanagerinterface.h | 5 +- src/libs/qmljs/qmljsplugindumper.cpp | 13 ++-- tests/auto/qml/codemodel/check/tst_check.cpp | 2 - .../dependencies/tst_dependencies.cpp | 2 - .../importscheck/tst_importscheck.cpp | 5 -- 6 files changed, 36 insertions(+), 62 deletions(-) diff --git a/src/libs/qmljs/qmljsmodelmanagerinterface.cpp b/src/libs/qmljs/qmljsmodelmanagerinterface.cpp index 86e1e7046e9..e89b8ae23c8 100644 --- a/src/libs/qmljs/qmljsmodelmanagerinterface.cpp +++ b/src/libs/qmljs/qmljsmodelmanagerinterface.cpp @@ -106,6 +106,7 @@ ModelManagerInterface::ModelManagerInterface(QObject *parent) m_defaultImportPaths(environmentImportPaths()), m_pluginDumper(new PluginDumper(this)) { + m_futureSynchronizer.setCancelOnWait(false); m_indexerDisabled = qEnvironmentVariableIsSet("QTC_NO_CODE_INDEXER"); m_updateCppQmlTypesTimer = new QTimer(this); @@ -307,20 +308,6 @@ void ModelManagerInterface::updateSourceFiles(const QStringList &files, refreshSourceFiles(files, emitDocumentOnDiskChanged); } -void ModelManagerInterface::cleanupFutures() -{ - QMutexLocker lock(&m_futuresMutex); - const int maxFutures = 10; - if (m_futures.size() > maxFutures) { - const QList> futures = m_futures; - m_futures.clear(); - for (const QFuture &future : futures) { - if (!(future.isFinished() || future.isCanceled())) - m_futures.append(future); - } - } -} - QFuture ModelManagerInterface::refreshSourceFiles(const QStringList &sourceFiles, bool emitDocumentOnDiskChanged) { @@ -355,9 +342,9 @@ QFuture ModelManagerInterface::refreshSourceFiles(const QStringList &sourc void ModelManagerInterface::fileChangedOnDisk(const QString &path) { - Utils::runAsync(&ModelManagerInterface::parse, + addFuture(Utils::runAsync(&ModelManagerInterface::parse, workingCopyInternal(), QStringList(path), - this, Dialect(Dialect::AnyLanguage), true); + this, Dialect(Dialect::AnyLanguage), true)); } void ModelManagerInterface::removeFiles(const QStringList &files) @@ -654,7 +641,7 @@ QList ModelManagerInterface::allProjectInfos bool ModelManagerInterface::isIdle() const { - return m_futures.isEmpty(); + return m_futureSynchronizer.isEmpty(); } void ModelManagerInterface::emitDocumentChangedOnDisk(Document::Ptr doc) @@ -1571,41 +1558,37 @@ void ModelManagerInterface::setDefaultVContext(const ViewerContext &vContext) m_defaultVContexts[vContext.language] = vContext; } +void ModelManagerInterface::joinAllThreads() +{ + while (true) { + FutureSynchronizer futureSynchronizer; + { + QMutexLocker locker(&m_futuresMutex); + futureSynchronizer = m_futureSynchronizer; + m_futureSynchronizer.clearFutures(); + } + if (futureSynchronizer.isEmpty()) + return; + } +} + void ModelManagerInterface::test_joinAllThreads() { - // Loop since futures can spawn more futures as they finish. while (true) { - QFuture f; - // get one future - { - QMutexLocker lock(&m_futuresMutex); - for (QFuture &future : m_futures) { - if (!future.isFinished() && !future.isCanceled()) { - f = future; - break; - } - } - } - if (!f.isFinished() && !f.isCanceled()) { - f.waitForFinished(); - - // Some futures trigger more futures from connected signals - // and in tests, we care about finishing all of these too. - QEventLoop().processEvents(); - } else { - break; - } + joinAllThreads(); + // In order to process all onFinished handlers of finished futures + QCoreApplication::processEvents(); + QMutexLocker lock(&m_futuresMutex); + // If handlers created new futures, repeat the loop + if (m_futureSynchronizer.isEmpty()) + return; } - m_futures.clear(); } void ModelManagerInterface::addFuture(const QFuture &future) { - { - QMutexLocker lock(&m_futuresMutex); - m_futures.append(future); - } - cleanupFutures(); + QMutexLocker lock(&m_futuresMutex); + m_futureSynchronizer.addFuture(future); } Document::Ptr ModelManagerInterface::ensuredGetDocumentForPath(const QString &filePath) diff --git a/src/libs/qmljs/qmljsmodelmanagerinterface.h b/src/libs/qmljs/qmljsmodelmanagerinterface.h index f6142255469..e31c69b84e4 100644 --- a/src/libs/qmljs/qmljsmodelmanagerinterface.h +++ b/src/libs/qmljs/qmljsmodelmanagerinterface.h @@ -32,6 +32,7 @@ #include #include +#include #include #include @@ -243,7 +244,7 @@ protected: void setDefaultProject(const ProjectInfo &pInfo, ProjectExplorer::Project *p); private: - void cleanupFutures(); + void joinAllThreads(); void iterateQrcFiles(ProjectExplorer::Project *project, QrcResourceSelector resources, const std::function &callback); @@ -280,7 +281,7 @@ private: PluginDumper *m_pluginDumper = nullptr; mutable QMutex m_futuresMutex; - QList> m_futures; + Utils::FutureSynchronizer m_futureSynchronizer; bool m_indexerDisabled = false; }; diff --git a/src/libs/qmljs/qmljsplugindumper.cpp b/src/libs/qmljs/qmljsplugindumper.cpp index a98f7550d62..376166559f3 100644 --- a/src/libs/qmljs/qmljsplugindumper.cpp +++ b/src/libs/qmljs/qmljsplugindumper.cpp @@ -300,9 +300,10 @@ void PluginDumper::qmlPluginTypeDumpDone(int exitCode) QLatin1String("')); future.reportFinished(&infos); }); + m_modelManager->addFuture(future); - auto finalFuture = Utils::onFinished(future, this, - [this, libraryInfo, privatePlugin, libraryPath] (const QFuture& future) { + Utils::onFinished(future, this, [this, libraryInfo, privatePlugin, libraryPath] + (const QFuture& future) { CppQmlTypesInfo infos = future.result(); LibraryInfo libInfo = libraryInfo; @@ -325,7 +326,6 @@ void PluginDumper::qmlPluginTypeDumpDone(int exitCode) m_modelManager->updateLibraryInfo(libraryPath, libInfo); }); - m_modelManager->addFuture(finalFuture); } else { libraryInfo.setPluginTypeInfoStatus(LibraryInfo::DumpDone); libraryInfo.updateFingerprint(); @@ -395,6 +395,7 @@ QFuture PluginDumper::loadQmlTypeDescription(c future.reportFinished(&result); }); + m_modelManager->addFuture(future); return future; } @@ -598,12 +599,12 @@ void PluginDumper::loadQmltypesFile(const QStringList &qmltypesFilePaths, const QString &libraryPath, QmlJS::LibraryInfo libraryInfo) { - auto future = Utils::onFinished(loadQmlTypeDescription(qmltypesFilePaths), this, [=](const QFuture &typesFuture) + Utils::onFinished(loadQmlTypeDescription(qmltypesFilePaths), this, [=](const QFuture &typesFuture) { PluginDumper::QmlTypeDescription typesResult = typesFuture.result(); if (!typesResult.dependencies.isEmpty()) { - auto depFuture = Utils::onFinished(loadDependencies(typesResult.dependencies, QSharedPointer>()), this, + Utils::onFinished(loadDependencies(typesResult.dependencies, QSharedPointer>()), this, [typesResult, libraryInfo, libraryPath, this] (const QFuture &loadFuture) { PluginDumper::DependencyInfo loadResult = loadFuture.result(); @@ -621,7 +622,6 @@ void PluginDumper::loadQmltypesFile(const QStringList &qmltypesFilePaths, typesResult.moduleApis, objects); m_modelManager->updateLibraryInfo(libraryPath, libInfo); }); - m_modelManager->addFuture(depFuture); } else { QmlJS::LibraryInfo libInfo = libraryInfo; prepareLibraryInfo(libInfo, libraryPath, typesResult.dependencies, @@ -630,7 +630,6 @@ void PluginDumper::loadQmltypesFile(const QStringList &qmltypesFilePaths, m_modelManager->updateLibraryInfo(libraryPath, libInfo); } }); - m_modelManager->addFuture(future); } void PluginDumper::runQmlDump(const QmlJS::ModelManagerInterface::ProjectInfo &info, diff --git a/tests/auto/qml/codemodel/check/tst_check.cpp b/tests/auto/qml/codemodel/check/tst_check.cpp index 35e49392dde..45d3d11880c 100644 --- a/tests/auto/qml/codemodel/check/tst_check.cpp +++ b/tests/auto/qml/codemodel/check/tst_check.cpp @@ -101,9 +101,7 @@ void tst_Check::initTestCase() lPaths.maybeInsert(Utils::FilePath::fromString(p), Dialect::Qml); ModelManagerInterface::importScan(result, ModelManagerInterface::workingCopy(), lPaths, modelManager, false); - QCoreApplication::processEvents(); modelManager->test_joinAllThreads(); - QCoreApplication::processEvents(); } static bool offsetComparator(const Message &lhs, const Message &rhs) diff --git a/tests/auto/qml/codemodel/dependencies/tst_dependencies.cpp b/tests/auto/qml/codemodel/dependencies/tst_dependencies.cpp index 5b6767151f3..e2c5edc6bfd 100644 --- a/tests/auto/qml/codemodel/dependencies/tst_dependencies.cpp +++ b/tests/auto/qml/codemodel/dependencies/tst_dependencies.cpp @@ -151,9 +151,7 @@ void tst_Dependencies::test() lPaths.maybeInsert(Utils::FilePath::fromString(p), Dialect::Qml); ModelManagerInterface::importScan(result, ModelManagerInterface::workingCopy(), lPaths, ModelManagerInterface::instance(), false); - QCoreApplication::processEvents(); ModelManagerInterface::instance()->test_joinAllThreads(); - QCoreApplication::processEvents(); TestData data = testData(filename); Document::MutablePtr doc = data.doc; int nExpectedSemanticMessages = data.semanticMessages; diff --git a/tests/auto/qml/codemodel/importscheck/tst_importscheck.cpp b/tests/auto/qml/codemodel/importscheck/tst_importscheck.cpp index f325338e4d7..0dc78ecd759 100644 --- a/tests/auto/qml/codemodel/importscheck/tst_importscheck.cpp +++ b/tests/auto/qml/codemodel/importscheck/tst_importscheck.cpp @@ -76,7 +76,6 @@ void scanDirectory(const QString &dir) paths.maybeInsert(Utils::FilePath::fromString(dir), Dialect::Qml); ModelManagerInterface::importScan(result, ModelManagerInterface::workingCopy(), paths, ModelManagerInterface::instance(), false); - QCoreApplication::processEvents(); ModelManagerInterface::instance()->test_joinAllThreads(); ViewerContext vCtx; vCtx.paths.append(dir); @@ -273,7 +272,6 @@ void tst_ImportCheck::importTypes() modelManager->activateScan(); modelManager->updateSourceFiles(QStringList(qmlFile), false); - QCoreApplication::processEvents(); modelManager->test_joinAllThreads(); Snapshot snapshot = modelManager->newestSnapshot(); @@ -288,7 +286,6 @@ void tst_ImportCheck::importTypes() return link(); }; getContext(); - QCoreApplication::processEvents(); modelManager->test_joinAllThreads(); snapshot = modelManager->newestSnapshot(); doc = snapshot.document(qmlFile); @@ -367,7 +364,6 @@ void tst_ImportCheck::moduleMapping() scanDirectory(qtQuickImportPath); modelManager->updateSourceFiles(QStringList(qmlFile), false); - QCoreApplication::processEvents(); modelManager->test_joinAllThreads(); Snapshot snapshot = modelManager->newestSnapshot(); @@ -383,7 +379,6 @@ void tst_ImportCheck::moduleMapping() return link(); }; getContext(); - QCoreApplication::processEvents(); modelManager->test_joinAllThreads(); snapshot = modelManager->newestSnapshot(); doc = snapshot.document(qmlFile);