From 29994ea8ef88b898521eadbb5f7a5444ab46f6c0 Mon Sep 17 00:00:00 2001 From: Joerg Bornemann Date: Wed, 28 Feb 2018 09:32:42 +0100 Subject: Guard QScriptEngine's creation/destruction with a mutex When building multiple configurations in one go we create one QScriptEngine per configuration, each in its own thread, but nearly at the same time. It turns out that QScriptEngine's creation/destruction is not thread-safe, which leads to all kinds of interesting effects in the multiple configurations case. We fix this by guarding creation/destruction of QScriptEngine with a mutex. Task-number: QBS-1308 Change-Id: Ie01733d5943f1fb89e1d25344c4ffa998095e394 Reviewed-by: Christian Kandeler Reviewed-by: Joerg Bornemann --- src/lib/corelib/buildgraph/jscommandexecutor.cpp | 2 +- .../corelib/buildgraph/rulesevaluationcontext.cpp | 2 +- src/lib/corelib/language/scriptengine.cpp | 11 ++++++++ src/lib/corelib/language/scriptengine.h | 5 +++- .../testdata/multiple-configurations/file.cpp | 6 +++++ .../testdata/multiple-configurations/file.h | 1 + .../testdata/multiple-configurations/lib.cpp | 1 + .../testdata/multiple-configurations/lib.h | 1 + .../testdata/multiple-configurations/main.cpp | 6 +++++ .../multiple-configurations.qbs | 15 +++++++++++ tests/auto/blackbox/tst_blackbox.cpp | 29 ++++++++++++++++++++++ tests/auto/blackbox/tst_blackbox.h | 1 + tests/auto/language/tst_language.cpp | 2 +- 13 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 tests/auto/blackbox/testdata/multiple-configurations/file.cpp create mode 100644 tests/auto/blackbox/testdata/multiple-configurations/file.h create mode 100644 tests/auto/blackbox/testdata/multiple-configurations/lib.cpp create mode 100644 tests/auto/blackbox/testdata/multiple-configurations/lib.h create mode 100644 tests/auto/blackbox/testdata/multiple-configurations/main.cpp create mode 100644 tests/auto/blackbox/testdata/multiple-configurations/multiple-configurations.qbs diff --git a/src/lib/corelib/buildgraph/jscommandexecutor.cpp b/src/lib/corelib/buildgraph/jscommandexecutor.cpp index 3b8e9ab42..1a7224afc 100644 --- a/src/lib/corelib/buildgraph/jscommandexecutor.cpp +++ b/src/lib/corelib/buildgraph/jscommandexecutor.cpp @@ -167,7 +167,7 @@ private: ScriptEngine *provideScriptEngine() { if (!m_scriptEngine) - m_scriptEngine = new ScriptEngine(m_logger, EvalContext::JsCommand, this); + m_scriptEngine = ScriptEngine::create(m_logger, EvalContext::JsCommand, this); return m_scriptEngine; } diff --git a/src/lib/corelib/buildgraph/rulesevaluationcontext.cpp b/src/lib/corelib/buildgraph/rulesevaluationcontext.cpp index 0b6774adb..c81e40c4f 100644 --- a/src/lib/corelib/buildgraph/rulesevaluationcontext.cpp +++ b/src/lib/corelib/buildgraph/rulesevaluationcontext.cpp @@ -55,7 +55,7 @@ namespace Internal { RulesEvaluationContext::RulesEvaluationContext(const Logger &logger) : m_logger(logger), - m_engine(new ScriptEngine(m_logger, EvalContext::RuleExecution)), + m_engine(ScriptEngine::create(m_logger, EvalContext::RuleExecution)), m_observer(nullptr), m_initScopeCalls(0) { diff --git a/src/lib/corelib/language/scriptengine.cpp b/src/lib/corelib/language/scriptengine.cpp index b1c6d1410..8f31b879f 100644 --- a/src/lib/corelib/language/scriptengine.cpp +++ b/src/lib/corelib/language/scriptengine.cpp @@ -97,6 +97,8 @@ uint qHash(const ScriptEngine::PropertyCacheKey &k, uint seed = 0) combineHash(qHash(k.m_propertyName), qHash(k.m_propertyMap), seed), seed); } +std::mutex ScriptEngine::m_creationDestructionMutex; + ScriptEngine::ScriptEngine(Logger &logger, EvalContext evalContext, QObject *parent) : QScriptEngine(parent), m_scriptImporter(new ScriptImporter(this)), m_modulePropertyScriptClass(nullptr), @@ -116,8 +118,17 @@ ScriptEngine::ScriptEngine(Logger &logger, EvalContext evalContext, QObject *par extendJavaScriptBuiltins(); } +ScriptEngine *ScriptEngine::create(Logger &logger, EvalContext evalContext, QObject *parent) +{ + std::lock_guard lock(m_creationDestructionMutex); + return new ScriptEngine(logger, evalContext, parent); +} + ScriptEngine::~ScriptEngine() { + m_creationDestructionMutex.lock(); + connect(this, &QObject::destroyed, std::bind(&std::mutex::unlock, &m_creationDestructionMutex)); + releaseResourcesOfScriptObjects(); delete (m_scriptImporter); if (m_elapsedTimeImporting != -1) { diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h index 05f4f519f..c01adbcbe 100644 --- a/src/lib/corelib/language/scriptengine.h +++ b/src/lib/corelib/language/scriptengine.h @@ -56,6 +56,7 @@ #include #include +#include #include #include #include @@ -96,8 +97,9 @@ enum class ObserveMode { Enabled, Disabled }; class QBS_AUTOTEST_EXPORT ScriptEngine : public QScriptEngine { Q_OBJECT -public: ScriptEngine(Logger &logger, EvalContext evalContext, QObject *parent = nullptr); +public: + static ScriptEngine *create(Logger &logger, EvalContext evalContext, QObject *parent = nullptr); ~ScriptEngine(); Logger &logger() const { return m_logger; } @@ -244,6 +246,7 @@ private: friend bool operator==(const PropertyCacheKey &lhs, const PropertyCacheKey &rhs); friend uint qHash(const ScriptEngine::PropertyCacheKey &k, uint seed); + static std::mutex m_creationDestructionMutex; ScriptImporter *m_scriptImporter; QScriptClass *m_modulePropertyScriptClass; QHash m_jsImportCache; diff --git a/tests/auto/blackbox/testdata/multiple-configurations/file.cpp b/tests/auto/blackbox/testdata/multiple-configurations/file.cpp new file mode 100644 index 000000000..44ebc2ee3 --- /dev/null +++ b/tests/auto/blackbox/testdata/multiple-configurations/file.cpp @@ -0,0 +1,6 @@ +#include + +void f() +{ + l(); +} diff --git a/tests/auto/blackbox/testdata/multiple-configurations/file.h b/tests/auto/blackbox/testdata/multiple-configurations/file.h new file mode 100644 index 000000000..789447c02 --- /dev/null +++ b/tests/auto/blackbox/testdata/multiple-configurations/file.h @@ -0,0 +1 @@ +void f(); diff --git a/tests/auto/blackbox/testdata/multiple-configurations/lib.cpp b/tests/auto/blackbox/testdata/multiple-configurations/lib.cpp new file mode 100644 index 000000000..9a6145659 --- /dev/null +++ b/tests/auto/blackbox/testdata/multiple-configurations/lib.cpp @@ -0,0 +1 @@ +void l() {} diff --git a/tests/auto/blackbox/testdata/multiple-configurations/lib.h b/tests/auto/blackbox/testdata/multiple-configurations/lib.h new file mode 100644 index 000000000..f8be99ced --- /dev/null +++ b/tests/auto/blackbox/testdata/multiple-configurations/lib.h @@ -0,0 +1 @@ +void l(); diff --git a/tests/auto/blackbox/testdata/multiple-configurations/main.cpp b/tests/auto/blackbox/testdata/multiple-configurations/main.cpp new file mode 100644 index 000000000..4fa5b1f97 --- /dev/null +++ b/tests/auto/blackbox/testdata/multiple-configurations/main.cpp @@ -0,0 +1,6 @@ +#include + +int main() +{ + f(); +} diff --git a/tests/auto/blackbox/testdata/multiple-configurations/multiple-configurations.qbs b/tests/auto/blackbox/testdata/multiple-configurations/multiple-configurations.qbs new file mode 100644 index 000000000..f8371e983 --- /dev/null +++ b/tests/auto/blackbox/testdata/multiple-configurations/multiple-configurations.qbs @@ -0,0 +1,15 @@ +import qbs + +Project { + StaticLibrary { + name: "lib" + Depends { name: "cpp" } + files: ["lib.cpp", "lib.h"] + } + CppApplication { + name: "app" + Depends { name: "lib" } + cpp.includePaths: project.sourceDirectory + files: ["file.cpp", "file.h", "main.cpp"] + } +} diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp index 8ef77c9b4..6ace7e674 100644 --- a/tests/auto/blackbox/tst_blackbox.cpp +++ b/tests/auto/blackbox/tst_blackbox.cpp @@ -3910,6 +3910,35 @@ void TestBlackbox::multipleChanges() QVERIFY(m_qbsStdout.contains("prop: true")); } +void TestBlackbox::multipleConfigurations() +{ + QDir::setCurrent(testDataDir + "/multiple-configurations"); + QbsRunParameters params(QStringList{"config:x", "config:y", "config:z"}); + params.profile.clear(); + struct DefaultProfileSwitcher + { + DefaultProfileSwitcher() + { + const SettingsPtr s = settings(); + oldDefaultProfile = s->defaultProfile(); + s->setValue("defaultProfile", profileName()); + s->sync(); + } + ~DefaultProfileSwitcher() + { + const SettingsPtr s = settings(); + s->setValue("defaultProfile", oldDefaultProfile); + s->sync(); + } + QVariant oldDefaultProfile; + }; + DefaultProfileSwitcher dps; + QCOMPARE(runQbs(params), 0); + QCOMPARE(m_qbsStdout.count("compiling lib.cpp"), 3); + QCOMPARE(m_qbsStdout.count("compiling file.cpp"), 3); + QCOMPARE(m_qbsStdout.count("compiling main.cpp"), 3); +} + void TestBlackbox::nestedGroups() { QDir::setCurrent(testDataDir + "/nested-groups"); diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h index 9cf76ab5a..d0f8ab7b3 100644 --- a/tests/auto/blackbox/tst_blackbox.h +++ b/tests/auto/blackbox/tst_blackbox.h @@ -153,6 +153,7 @@ private slots: void missingOverridePrefix(); void movedFileDependency(); void multipleChanges(); + void multipleConfigurations(); void nestedGroups(); void nestedProperties(); void newOutputArtifact(); diff --git a/tests/auto/language/tst_language.cpp b/tests/auto/language/tst_language.cpp index 35f18732c..f1e21cc26 100644 --- a/tests/auto/language/tst_language.cpp +++ b/tests/auto/language/tst_language.cpp @@ -174,7 +174,7 @@ void TestLanguage::init() void TestLanguage::initTestCase() { m_logger = Logger(m_logSink); - m_engine = new ScriptEngine(m_logger, EvalContext::PropertyEvaluation, this); + m_engine = ScriptEngine::create(m_logger, EvalContext::PropertyEvaluation, this); loader = new Loader(m_engine, m_logger); loader->setSearchPaths(QStringList() << QLatin1String(SRCDIR "/../../../share/qbs")); -- cgit v1.2.1