From 4dbfd3bfd936702e58ac90cc102d7df8bd4fe5a8 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Tue, 8 Dec 2015 10:49:10 -0800 Subject: [PATCH 1/7] Fix concurrency issues for bools in ScriptEngine Several member variables were unsafely being accessed from multiple threads. Making these variables std::atomic makes this safe. --- libraries/script-engine/src/ScriptEngine.cpp | 2 +- libraries/script-engine/src/ScriptEngine.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 3306659c80..daadb0e394 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -183,7 +183,7 @@ void ScriptEngine::runInThread() { QSet ScriptEngine::_allKnownScriptEngines; QMutex ScriptEngine::_allScriptsMutex; -bool ScriptEngine::_stoppingAllScripts = false; +std::atomic ScriptEngine::_stoppingAllScripts = false; void ScriptEngine::stopAllScripts(QObject* application) { _allScriptsMutex.lock(); diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 90b99d46fd..82b91aed2e 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -166,8 +166,8 @@ signals: protected: QString _scriptContents; QString _parentURL; - bool _isFinished { false }; - bool _isRunning { false }; + std::atomic _isFinished { false }; + std::atomic _isRunning { false }; int _evaluatesPending { 0 }; bool _isInitialized { false }; QHash _timerFunctionMap; @@ -206,7 +206,7 @@ protected: static QSet _allKnownScriptEngines; static QMutex _allScriptsMutex; - static bool _stoppingAllScripts; + static std::atomic _stoppingAllScripts; }; #endif // hifi_ScriptEngine_h \ No newline at end of file From 4bfce768ca68951d44d2f973d1bd408f46b5133c Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 9 Dec 2015 14:48:33 -0800 Subject: [PATCH 2/7] Add locks around read/writes of _scriptEnginesHash --- interface/src/Application.cpp | 113 +++++++++++++++++++++++----------- interface/src/Application.h | 1 + 2 files changed, 77 insertions(+), 37 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 0b29cdc892..8655d6b77e 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4259,10 +4259,13 @@ ScriptEngine* Application::loadScript(const QString& scriptFilename, bool isUser QUrl scriptUrl(scriptFilename); const QString& scriptURLString = scriptUrl.toString(); - if (_scriptEnginesHash.contains(scriptURLString) && loadScriptFromEditor - && !_scriptEnginesHash[scriptURLString]->isFinished()) { + { + QReadLocker lock(&_scriptEnginesHashLock); + if (_scriptEnginesHash.contains(scriptURLString) && loadScriptFromEditor + && !_scriptEnginesHash[scriptURLString]->isFinished()) { - return _scriptEnginesHash[scriptURLString]; + return _scriptEnginesHash[scriptURLString]; + } } ScriptEngine* scriptEngine = new ScriptEngine(NO_SCRIPT, "", &_controllerScriptingInterface); @@ -4302,7 +4305,11 @@ void Application::reloadScript(const QString& scriptName, bool isUserLoaded) { void Application::handleScriptEngineLoaded(const QString& scriptFilename) { ScriptEngine* scriptEngine = qobject_cast(sender()); - _scriptEnginesHash.insertMulti(scriptFilename, scriptEngine); + { + QWriteLocker lock(&_scriptEnginesHashLock); + _scriptEnginesHash.insertMulti(scriptFilename, scriptEngine); + } + _runningScriptsWidget->setRunningScripts(getRunningScripts()); UserActivityLogger::getInstance().loadedScript(scriptFilename); @@ -4317,55 +4324,86 @@ void Application::handleScriptLoadError(const QString& scriptFilename) { QMessageBox::warning(getWindow(), "Error Loading Script", scriptFilename + " failed to load."); } -void Application::scriptFinished(const QString& scriptName) { - const QString& scriptURLString = QUrl(scriptName).toString(); - QHash::iterator it = _scriptEnginesHash.find(scriptURLString); - if (it != _scriptEnginesHash.end()) { - _scriptEnginesHash.erase(it); - _runningScriptsWidget->scriptStopped(scriptName); - _runningScriptsWidget->setRunningScripts(getRunningScripts()); - } +QStringList Application::getRunningScripts() { + QReadLocker lock(&_scriptEnginesHashLock); + return _scriptEnginesHash.keys(); } -void Application::stopAllScripts(bool restart) { - if (restart) { - // Delete all running scripts from cache so that they are re-downloaded when they are restarted - auto scriptCache = DependencyManager::get(); - for (QHash::const_iterator it = _scriptEnginesHash.constBegin(); - it != _scriptEnginesHash.constEnd(); it++) { - if (!it.value()->isFinished()) { - scriptCache->deleteScript(it.key()); +ScriptEngine* Application::getScriptEngine(const QString& scriptHash) { + QReadLocker lock(&_scriptEnginesHashLock); + return _scriptEnginesHash.value(scriptHash, nullptr); +} + +void Application::scriptFinished(const QString& scriptName, ScriptEngine* engine) { + bool removed = false; + { + QWriteLocker lock(&_scriptEnginesHashLock); + const QString& scriptURLString = QUrl(scriptName).toString(); + for (auto it = _scriptEnginesHash.find(scriptURLString); it != _scriptEnginesHash.end(); ++it) { + if (it.value() == engine) { + _scriptEnginesHash.erase(it); + removed = true; + break; } } } + postLambdaEvent([this, scriptName]() { + _runningScriptsWidget->scriptStopped(scriptName); + _runningScriptsWidget->setRunningScripts(getRunningScripts()); + }); +} - // Stop and possibly restart all currently running scripts - for (QHash::const_iterator it = _scriptEnginesHash.constBegin(); +void Application::stopAllScripts(bool restart) { + { + QReadLocker lock(&_scriptEnginesHashLock); + + if (restart) { + // Delete all running scripts from cache so that they are re-downloaded when they are restarted + auto scriptCache = DependencyManager::get(); + for (QHash::const_iterator it = _scriptEnginesHash.constBegin(); it != _scriptEnginesHash.constEnd(); it++) { - if (it.value()->isFinished()) { - continue; + if (!it.value()->isFinished()) { + scriptCache->deleteScript(it.key()); + } + } } - if (restart && it.value()->isUserLoaded()) { - connect(it.value(), SIGNAL(finished(const QString&)), SLOT(reloadScript(const QString&))); + + // Stop and possibly restart all currently running scripts + for (QHash::const_iterator it = _scriptEnginesHash.constBegin(); + it != _scriptEnginesHash.constEnd(); it++) { + if (it.value()->isFinished()) { + continue; + } + if (restart && it.value()->isUserLoaded()) { + connect(it.value(), &ScriptEngine::finished, this, [this](QString scriptName, ScriptEngine* engine) { + reloadScript(scriptName); + }); + } + QMetaObject::invokeMethod(it.value(), "stop"); + //it.value()->stop(); + qCDebug(interfaceapp) << "stopping script..." << it.key(); } - it.value()->stop(); - qCDebug(interfaceapp) << "stopping script..." << it.key(); } getMyAvatar()->clearScriptableSettings(); } bool Application::stopScript(const QString& scriptHash, bool restart) { bool stoppedScript = false; - if (_scriptEnginesHash.contains(scriptHash)) { - ScriptEngine* scriptEngine = _scriptEnginesHash[scriptHash]; - if (restart) { - auto scriptCache = DependencyManager::get(); - scriptCache->deleteScript(QUrl(scriptHash)); - connect(scriptEngine, SIGNAL(finished(const QString&)), SLOT(reloadScript(const QString&))); + { + QReadLocker lock(&_scriptEnginesHashLock); + if (_scriptEnginesHash.contains(scriptHash)) { + ScriptEngine* scriptEngine = _scriptEnginesHash[scriptHash]; + if (restart) { + auto scriptCache = DependencyManager::get(); + scriptCache->deleteScript(QUrl(scriptHash)); + connect(scriptEngine, &ScriptEngine::finished, this, [this](QString scriptName, ScriptEngine* engine) { + reloadScript(scriptName); + }); + } + scriptEngine->stop(); + stoppedScript = true; + qCDebug(interfaceapp) << "stopping script..." << scriptHash; } - scriptEngine->stop(); - stoppedScript = true; - qCDebug(interfaceapp) << "stopping script..." << scriptHash; } if (_scriptEnginesHash.empty()) { getMyAvatar()->clearScriptableSettings(); @@ -4384,6 +4422,7 @@ void Application::reloadOneScript(const QString& scriptName) { } void Application::loadDefaultScripts() { + QReadLocker lock(&_scriptEnginesHashLock); if (!_scriptEnginesHash.contains(DEFAULT_SCRIPTS_JS_URL)) { loadScript(DEFAULT_SCRIPTS_JS_URL); } diff --git a/interface/src/Application.h b/interface/src/Application.h index ce33f051ef..2f4e33e52d 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -505,6 +505,7 @@ private: TouchEvent _lastTouchEvent; + QReadWriteLock _scriptEnginesHashLock; RunningScriptsWidget* _runningScriptsWidget; QHash _scriptEnginesHash; bool _runningScriptsWidgetWasVisible; From fc6a78d9ef74483a861aea122fee0885c674628f Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 9 Dec 2015 14:49:05 -0800 Subject: [PATCH 3/7] Update scriptFinished signal to be a DirectConnection --- interface/src/Application.cpp | 2 +- interface/src/Application.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 8655d6b77e..740b0062f0 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4013,7 +4013,7 @@ void Application::registerScriptEngineWithApplicationServices(ScriptEngine* scri scriptEngine->registerGlobalObject("Clipboard", clipboardScriptable); connect(scriptEngine, SIGNAL(finished(const QString&)), clipboardScriptable, SLOT(deleteLater())); - connect(scriptEngine, SIGNAL(finished(const QString&)), this, SLOT(scriptFinished(const QString&))); + connect(scriptEngine, &ScriptEngine::finished, this, &Application::scriptFinished, Qt::DirectConnection); connect(scriptEngine, SIGNAL(loadScript(const QString&, bool)), this, SLOT(loadScript(const QString&, bool))); connect(scriptEngine, SIGNAL(reloadScript(const QString&, bool)), this, SLOT(reloadScript(const QString&, bool))); diff --git a/interface/src/Application.h b/interface/src/Application.h index 2f4e33e52d..0d5b920c10 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -202,8 +202,8 @@ public: NodeToJurisdictionMap& getEntityServerJurisdictions() { return _entityServerJurisdictions; } - QStringList getRunningScripts() { return _scriptEnginesHash.keys(); } - ScriptEngine* getScriptEngine(const QString& scriptHash) { return _scriptEnginesHash.value(scriptHash, NULL); } + QStringList getRunningScripts(); + ScriptEngine* getScriptEngine(const QString& scriptHash); float getRenderResolutionScale() const; @@ -336,7 +336,7 @@ private slots: void loadSettings(); void saveSettings(); - void scriptFinished(const QString& scriptName); + void scriptFinished(const QString& scriptName, ScriptEngine* engine); void saveScripts(); void reloadScript(const QString& scriptName, bool isUserLoaded = true); From e7a1550b64e831a9b0eb820d277c72e324c8b189 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 9 Dec 2015 14:50:09 -0800 Subject: [PATCH 4/7] Update ScriptEngine::finished to include --- libraries/script-engine/src/ScriptEngine.cpp | 4 ++-- libraries/script-engine/src/ScriptEngine.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index daadb0e394..42287bbea7 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -183,7 +183,7 @@ void ScriptEngine::runInThread() { QSet ScriptEngine::_allKnownScriptEngines; QMutex ScriptEngine::_allScriptsMutex; -std::atomic ScriptEngine::_stoppingAllScripts = false; +std::atomic ScriptEngine::_stoppingAllScripts { false }; void ScriptEngine::stopAllScripts(QObject* application) { _allScriptsMutex.lock(); @@ -752,7 +752,7 @@ void ScriptEngine::run() { } if (_wantSignals) { - emit finished(_fileNameString); + emit finished(_fileNameString, this); } _isRunning = false; diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 82b91aed2e..f8da05135a 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -153,7 +153,7 @@ signals: void errorLoadingScript(const QString& scriptFilename); void update(float deltaTime); void scriptEnding(); - void finished(const QString& fileNameString); + void finished(const QString& fileNameString, ScriptEngine* engine); void cleanupMenuItem(const QString& menuItemString); void printedMessage(const QString& message); void errorMessage(const QString& message); From a21e07327d4da97256411cb6be691f13cedb8af2 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 9 Dec 2015 14:50:30 -0800 Subject: [PATCH 5/7] Update ScriptEngine::stop() to always run on its thread --- libraries/script-engine/src/ScriptEngine.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 42287bbea7..e06fe2b209 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -775,6 +775,10 @@ void ScriptEngine::stopAllTimers() { void ScriptEngine::stop() { if (!_isFinished) { + if (QThread::currentThread() != thread()) { + QMetaObject::invokeMethod(this, "stop"); + return; + } _isFinished = true; if (_wantSignals) { emit runningStateChanged(); From 5f60aeef2217e5fc1e3be0da0754a80982f933a4 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 9 Dec 2015 14:50:48 -0800 Subject: [PATCH 6/7] Update ScriptEngine::_isUserLoaded to be atomic --- libraries/script-engine/src/ScriptEngine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index f8da05135a..67a206d673 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -193,7 +193,7 @@ protected: Quat _quatLibrary; Vec3 _vec3Library; ScriptUUID _uuidLibrary; - bool _isUserLoaded { false }; + std::atomic _isUserLoaded { false }; bool _isReloading { false }; ArrayBufferClass* _arrayBufferClass; From ad0865171b53ca9030cf6875ae8afced602a2b39 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 9 Dec 2015 15:52:30 -0800 Subject: [PATCH 7/7] Fix unused variable in scriptFinished --- interface/src/Application.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 740b0062f0..d648537155 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4347,10 +4347,12 @@ void Application::scriptFinished(const QString& scriptName, ScriptEngine* engine } } } - postLambdaEvent([this, scriptName]() { - _runningScriptsWidget->scriptStopped(scriptName); - _runningScriptsWidget->setRunningScripts(getRunningScripts()); - }); + if (removed) { + postLambdaEvent([this, scriptName]() { + _runningScriptsWidget->scriptStopped(scriptName); + _runningScriptsWidget->setRunningScripts(getRunningScripts()); + }); + } } void Application::stopAllScripts(bool restart) {