From a0d80b9508fe4ebd79dc030f832a410d3e25e73f Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 9 May 2016 16:23:59 -0700 Subject: [PATCH 1/8] Fix hang on new script on shutdown --- libraries/script-engine/src/ScriptEngines.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index c6070e0598..d78fd9fe0c 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -118,9 +118,9 @@ void ScriptEngines::registerScriptInitializer(ScriptInitializer initializer) { } void ScriptEngines::addScriptEngine(ScriptEngine* engine) { - _allScriptsMutex.lock(); - _allKnownScriptEngines.insert(engine); - _allScriptsMutex.unlock(); + if (!_stoppingAllScripts) { + _allKnownScriptEngines.insert(engine); + } } void ScriptEngines::removeScriptEngine(ScriptEngine* engine) { @@ -128,16 +128,15 @@ void ScriptEngines::removeScriptEngine(ScriptEngine* engine) { // from the list of running scripts. We don't do this if we're in the process of stopping all scripts // because that method removes scripts from its list as it iterates them if (!_stoppingAllScripts) { - _allScriptsMutex.lock(); + QMutexLocker locker(&_allScriptsMutex); _allKnownScriptEngines.remove(engine); - _allScriptsMutex.unlock(); } } void ScriptEngines::shutdownScripting() { - _allScriptsMutex.lock(); _stoppingAllScripts = true; ScriptEngine::_stoppingAllScripts = true; + QMutexLocker locker(&_allScriptsMutex); qCDebug(scriptengine) << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size(); QMutableSetIterator i(_allKnownScriptEngines); @@ -173,8 +172,6 @@ void ScriptEngines::shutdownScripting() { i.remove(); } } - _stoppingAllScripts = false; - _allScriptsMutex.unlock(); qCDebug(scriptengine) << "DONE Stopping all scripts...."; } From 694dc1bbf6e0cf4676da89aa7076079cb9c37b99 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 9 May 2016 17:28:27 -0700 Subject: [PATCH 2/8] Delay Overlays cleanup for scripting --- interface/src/Application.cpp | 3 +++ interface/src/ui/overlays/Overlays.cpp | 11 ++--------- interface/src/ui/overlays/Overlays.h | 4 ++-- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 5b0d5c65ce..77e266bdb8 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -1189,6 +1189,9 @@ void Application::cleanupBeforeQuit() { DependencyManager::get()->shutdownScripting(); // stop all currently running global scripts DependencyManager::destroy(); + // Cleanup all overlays after the scripts, as scripts might add more + _overlays.cleanupAllOverlays(); + // first stop all timers directly or by invokeMethod // depending on what thread they run in locationUpdateTimer.stop(); diff --git a/interface/src/ui/overlays/Overlays.cpp b/interface/src/ui/overlays/Overlays.cpp index 1179fbaa50..9ff7f6268f 100644 --- a/interface/src/ui/overlays/Overlays.cpp +++ b/interface/src/ui/overlays/Overlays.cpp @@ -36,15 +36,8 @@ #include -Overlays::Overlays() : _nextOverlayID(1) { - connect(qApp, &Application::beforeAboutToQuit, [=] { - cleanupAllOverlays(); - }); -} - -Overlays::~Overlays() { -} - +Overlays::Overlays() : + _nextOverlayID(1) {} void Overlays::cleanupAllOverlays() { { diff --git a/interface/src/ui/overlays/Overlays.h b/interface/src/ui/overlays/Overlays.h index 25ba00fdf0..f47f8de153 100644 --- a/interface/src/ui/overlays/Overlays.h +++ b/interface/src/ui/overlays/Overlays.h @@ -62,7 +62,6 @@ class Overlays : public QObject { public: Overlays(); - ~Overlays(); void init(); void update(float deltatime); @@ -73,6 +72,8 @@ public: Overlay::Pointer getOverlay(unsigned int id) const; OverlayPanel::Pointer getPanel(unsigned int id) const { return _panels[id]; } + void cleanupAllOverlays(); + public slots: /// adds an overlay with the specific properties unsigned int addOverlay(const QString& type, const QVariant& properties); @@ -145,7 +146,6 @@ signals: private: void cleanupOverlaysToDelete(); - void cleanupAllOverlays(); QMap _overlaysHUD; QMap _overlaysWorld; From 2d820221dc83ffb42ac2070793393334e8f28d15 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 9 May 2016 17:30:44 -0700 Subject: [PATCH 3/8] Clear queued processing on quit --- interface/src/Application.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 77e266bdb8..de955bb5b7 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -1185,6 +1185,9 @@ void Application::cleanupBeforeQuit() { getEntities()->shutdown(); // tell the entities system we're shutting down, so it will stop running scripts + // Clear any queued processing (I/O, FBX/OBJ/Texture parsing) + QThreadPool::globalInstance()->clear(); + DependencyManager::get()->saveScripts(); DependencyManager::get()->shutdownScripting(); // stop all currently running global scripts DependencyManager::destroy(); From 68731973d8f8288635b2e2aa2bc611bdd74c26e0 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Mon, 9 May 2016 18:04:38 -0700 Subject: [PATCH 4/8] Keep locker when adding script engine --- libraries/script-engine/src/ScriptEngines.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index d78fd9fe0c..61220bc00b 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -119,6 +119,7 @@ void ScriptEngines::registerScriptInitializer(ScriptInitializer initializer) { void ScriptEngines::addScriptEngine(ScriptEngine* engine) { if (!_stoppingAllScripts) { + QMutexLocker locker(&_allScriptsMutex); _allKnownScriptEngines.insert(engine); } } From 70d0ebb91c94e59f47631f1e01ba3d89ddbf8c54 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 13 May 2016 15:24:15 -0700 Subject: [PATCH 5/8] Consolidate stoppingAllScripts to ScriptEngines::_stopped --- libraries/script-engine/src/ScriptEngine.cpp | 16 +++++++--------- libraries/script-engine/src/ScriptEngine.h | 3 --- libraries/script-engine/src/ScriptEngines.cpp | 7 +++---- libraries/script-engine/src/ScriptEngines.h | 3 +-- 4 files changed, 11 insertions(+), 18 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 6c7739c784..48635df324 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -52,8 +52,6 @@ #include "MIDIEvent.h" -std::atomic ScriptEngine::_stoppingAllScripts { false }; - static const QString SCRIPT_EXCEPTION_FORMAT = "[UncaughtException] %1 in %2:%3"; Q_DECLARE_METATYPE(QScriptEngine::FunctionSignature) @@ -655,7 +653,7 @@ void ScriptEngine::addEventHandler(const EntityItemID& entityID, const QString& QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fileName, int lineNumber) { - if (_stoppingAllScripts) { + if (DependencyManager::get()->_stopped) { return QScriptValue(); // bail early } @@ -691,7 +689,7 @@ QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fi } void ScriptEngine::run() { - if (_stoppingAllScripts) { + if (DependencyManager::get()->_stopped) { return; // bail early - avoid setting state in init(), as evaluate() will bail too } @@ -901,7 +899,7 @@ QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int } QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) { - if (_stoppingAllScripts) { + if (DependencyManager::get()->_stopped) { qCDebug(scriptengine) << "Script.setInterval() while shutting down is ignored... parent script:" << getFilename(); return NULL; // bail early } @@ -910,7 +908,7 @@ QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) } QObject* ScriptEngine::setTimeout(const QScriptValue& function, int timeoutMS) { - if (_stoppingAllScripts) { + if (DependencyManager::get()->_stopped) { qCDebug(scriptengine) << "Script.setTimeout() while shutting down is ignored... parent script:" << getFilename(); return NULL; // bail early } @@ -962,7 +960,7 @@ void ScriptEngine::print(const QString& message) { // If no callback is specified, the included files will be loaded synchronously and will block execution until // all of the files have finished loading. void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callback) { - if (_stoppingAllScripts) { + if (DependencyManager::get()->_stopped) { qCDebug(scriptengine) << "Script.include() while shutting down is ignored..." << "includeFiles:" << includeFiles << "parent script:" << getFilename(); return; // bail early @@ -1063,7 +1061,7 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac } void ScriptEngine::include(const QString& includeFile, QScriptValue callback) { - if (_stoppingAllScripts) { + if (DependencyManager::get()->_stopped) { qCDebug(scriptengine) << "Script.include() while shutting down is ignored... " << "includeFile:" << includeFile << "parent script:" << getFilename(); return; // bail early @@ -1078,7 +1076,7 @@ void ScriptEngine::include(const QString& includeFile, QScriptValue callback) { // as a stand-alone script. To accomplish this, the ScriptEngine class just emits a signal which // the Application or other context will connect to in order to know to actually load the script void ScriptEngine::load(const QString& loadFile) { - if (_stoppingAllScripts) { + if (DependencyManager::get()->_stopped) { qCDebug(scriptengine) << "Script.load() while shutting down is ignored... " << "loadFile:" << loadFile << "parent script:" << getFilename(); return; // bail early diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 175a3f1f1c..abd5ed51f4 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -222,9 +222,6 @@ protected: QUrl currentSandboxURL {}; // The toplevel url string for the entity script that loaded the code being executed, else empty. void doWithEnvironment(const EntityItemID& entityID, const QUrl& sandboxURL, std::function operation); void callWithEnvironment(const EntityItemID& entityID, const QUrl& sandboxURL, QScriptValue function, QScriptValue thisObject, QScriptValueList args); - - friend class ScriptEngines; - static std::atomic _stoppingAllScripts; }; #endif // hifi_ScriptEngine_h diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index 61220bc00b..d519af1bce 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -118,7 +118,7 @@ void ScriptEngines::registerScriptInitializer(ScriptInitializer initializer) { } void ScriptEngines::addScriptEngine(ScriptEngine* engine) { - if (!_stoppingAllScripts) { + if (!_stopped) { QMutexLocker locker(&_allScriptsMutex); _allKnownScriptEngines.insert(engine); } @@ -128,15 +128,14 @@ void ScriptEngines::removeScriptEngine(ScriptEngine* engine) { // If we're not already in the middle of stopping all scripts, then we should remove ourselves // from the list of running scripts. We don't do this if we're in the process of stopping all scripts // because that method removes scripts from its list as it iterates them - if (!_stoppingAllScripts) { + if (!_stopped) { QMutexLocker locker(&_allScriptsMutex); _allKnownScriptEngines.remove(engine); } } void ScriptEngines::shutdownScripting() { - _stoppingAllScripts = true; - ScriptEngine::_stoppingAllScripts = true; + _stopped = true; QMutexLocker locker(&_allScriptsMutex); qCDebug(scriptengine) << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size(); diff --git a/libraries/script-engine/src/ScriptEngines.h b/libraries/script-engine/src/ScriptEngines.h index 0963b21600..5b4a21be66 100644 --- a/libraries/script-engine/src/ScriptEngines.h +++ b/libraries/script-engine/src/ScriptEngines.h @@ -87,17 +87,16 @@ protected: void onScriptEngineError(const QString& scriptFilename); void launchScriptEngine(ScriptEngine* engine); - Setting::Handle _firstRun { "firstRun", true }; QReadWriteLock _scriptEnginesHashLock; QHash _scriptEnginesHash; QSet _allKnownScriptEngines; QMutex _allScriptsMutex; - std::atomic _stoppingAllScripts { false }; std::list _scriptInitializers; mutable Setting::Handle _scriptsLocationHandle; ScriptsModel _scriptsModel; ScriptsModelFilter _scriptsModelFilter; + std::atomic _stopped { false }; }; QUrl normalizeScriptURL(const QUrl& rawScriptURL); From 13610b1220cc54e261596f02e36ff05ec5fbfc21 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 13 May 2016 15:24:31 -0700 Subject: [PATCH 6/8] Delete late-added script engines --- libraries/script-engine/src/ScriptEngines.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index d519af1bce..fc2e7a3eaf 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -118,7 +118,9 @@ void ScriptEngines::registerScriptInitializer(ScriptInitializer initializer) { } void ScriptEngines::addScriptEngine(ScriptEngine* engine) { - if (!_stopped) { + if (_stopped) { + engine->deleteLater(); + } else { QMutexLocker locker(&_allScriptsMutex); _allKnownScriptEngines.insert(engine); } From 36565598a786b40f0b9de5f76347efa418361ad1 Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 13 May 2016 15:52:53 -0700 Subject: [PATCH 7/8] Hide script stop behind accessor and fix friendship --- libraries/script-engine/src/ScriptEngine.cpp | 14 +++++++------- libraries/script-engine/src/ScriptEngine.h | 6 ++++-- libraries/script-engine/src/ScriptEngines.cpp | 9 ++++++--- libraries/script-engine/src/ScriptEngines.h | 3 ++- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 90434c977c..df31ad59f1 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -754,7 +754,7 @@ void ScriptEngine::addEventHandler(const EntityItemID& entityID, const QString& QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fileName, int lineNumber) { - if (DependencyManager::get()->_stopped) { + if (DependencyManager::get()->isStopped()) { return QScriptValue(); // bail early } @@ -790,7 +790,7 @@ QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fi } void ScriptEngine::run() { - if (DependencyManager::get()->_stopped) { + if (DependencyManager::get()->isStopped()) { return; // bail early - avoid setting state in init(), as evaluate() will bail too } @@ -1023,7 +1023,7 @@ QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int } QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) { - if (DependencyManager::get()->_stopped) { + if (DependencyManager::get()->isStopped()) { qCDebug(scriptengine) << "Script.setInterval() while shutting down is ignored... parent script:" << getFilename(); return NULL; // bail early } @@ -1032,7 +1032,7 @@ QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) } QObject* ScriptEngine::setTimeout(const QScriptValue& function, int timeoutMS) { - if (DependencyManager::get()->_stopped) { + if (DependencyManager::get()->isStopped()) { qCDebug(scriptengine) << "Script.setTimeout() while shutting down is ignored... parent script:" << getFilename(); return NULL; // bail early } @@ -1084,7 +1084,7 @@ void ScriptEngine::print(const QString& message) { // If no callback is specified, the included files will be loaded synchronously and will block execution until // all of the files have finished loading. void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callback) { - if (DependencyManager::get()->_stopped) { + if (DependencyManager::get()->isStopped()) { qCDebug(scriptengine) << "Script.include() while shutting down is ignored..." << "includeFiles:" << includeFiles << "parent script:" << getFilename(); return; // bail early @@ -1182,7 +1182,7 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac } void ScriptEngine::include(const QString& includeFile, QScriptValue callback) { - if (DependencyManager::get()->_stopped) { + if (DependencyManager::get()->isStopped()) { qCDebug(scriptengine) << "Script.include() while shutting down is ignored... " << "includeFile:" << includeFile << "parent script:" << getFilename(); return; // bail early @@ -1197,7 +1197,7 @@ void ScriptEngine::include(const QString& includeFile, QScriptValue callback) { // as a stand-alone script. To accomplish this, the ScriptEngine class just emits a signal which // the Application or other context will connect to in order to know to actually load the script void ScriptEngine::load(const QString& loadFile) { - if (DependencyManager::get()->_stopped) { + if (DependencyManager::get()->isStopped()) { qCDebug(scriptengine) << "Script.load() while shutting down is ignored... " << "loadFile:" << loadFile << "parent script:" << getFilename(); return; // bail early diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 8a3acb0fcb..993b4e1e8a 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -83,6 +83,10 @@ public: /// run the script in the callers thread, exit when stop() is called. void run(); + void waitTillDoneRunning(); + + QString getFilename() const; + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // NOTE - these are NOT intended to be public interfaces available to scripts, the are only Q_INVOKABLE so we can // properly ensure they are only called on the correct thread @@ -199,8 +203,6 @@ protected: qint64 _lastUpdate; void init(); - QString getFilename() const; - void waitTillDoneRunning(); bool evaluatePending() const { return _evaluatesPending > 0; } void timerFired(); void stopAllTimers(); diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index f2b5f1cd01..2dce6457f1 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -119,7 +119,7 @@ void ScriptEngines::registerScriptInitializer(ScriptInitializer initializer) { } void ScriptEngines::addScriptEngine(ScriptEngine* engine) { - if (_stopped) { + if (_isStopped) { engine->deleteLater(); } else { QMutexLocker locker(&_allScriptsMutex); @@ -131,14 +131,14 @@ void ScriptEngines::removeScriptEngine(ScriptEngine* engine) { // If we're not already in the middle of stopping all scripts, then we should remove ourselves // from the list of running scripts. We don't do this if we're in the process of stopping all scripts // because that method removes scripts from its list as it iterates them - if (!_stopped) { + if (!_isStopped) { QMutexLocker locker(&_allScriptsMutex); _allKnownScriptEngines.remove(engine); } } void ScriptEngines::shutdownScripting() { - _stopped = true; + _isStopped = true; QMutexLocker locker(&_allScriptsMutex); qCDebug(scriptengine) << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size(); @@ -498,6 +498,9 @@ void ScriptEngines::launchScriptEngine(ScriptEngine* scriptEngine) { } } +bool ScriptEngines::isStopped() const { + return _isStopped; +} void ScriptEngines::onScriptFinished(const QString& rawScriptURL, ScriptEngine* engine) { bool removed = false; diff --git a/libraries/script-engine/src/ScriptEngines.h b/libraries/script-engine/src/ScriptEngines.h index 8a395052a0..9a0a52bb52 100644 --- a/libraries/script-engine/src/ScriptEngines.h +++ b/libraries/script-engine/src/ScriptEngines.h @@ -86,6 +86,7 @@ protected: void onScriptEngineLoaded(const QString& scriptFilename); void onScriptEngineError(const QString& scriptFilename); void launchScriptEngine(ScriptEngine* engine); + bool isStopped() const; QReadWriteLock _scriptEnginesHashLock; QHash _scriptEnginesHash; @@ -95,7 +96,7 @@ protected: mutable Setting::Handle _scriptsLocationHandle; ScriptsModel _scriptsModel; ScriptsModelFilter _scriptsModelFilter; - std::atomic _stopped { false }; + std::atomic _isStopped { false }; }; QUrl normalizeScriptURL(const QUrl& rawScriptURL); From 4e62d7ff6114e7c529cd6307d8229545f2d279bc Mon Sep 17 00:00:00 2001 From: Zach Pomerantz Date: Fri, 13 May 2016 17:28:21 -0700 Subject: [PATCH 8/8] Define ScriptEngines::isStopped inline --- libraries/script-engine/src/ScriptEngines.cpp | 4 ---- libraries/script-engine/src/ScriptEngines.h | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngines.cpp b/libraries/script-engine/src/ScriptEngines.cpp index 2dce6457f1..805ddfdada 100644 --- a/libraries/script-engine/src/ScriptEngines.cpp +++ b/libraries/script-engine/src/ScriptEngines.cpp @@ -498,10 +498,6 @@ void ScriptEngines::launchScriptEngine(ScriptEngine* scriptEngine) { } } -bool ScriptEngines::isStopped() const { - return _isStopped; -} - void ScriptEngines::onScriptFinished(const QString& rawScriptURL, ScriptEngine* engine) { bool removed = false; { diff --git a/libraries/script-engine/src/ScriptEngines.h b/libraries/script-engine/src/ScriptEngines.h index 9a0a52bb52..72bf7d529e 100644 --- a/libraries/script-engine/src/ScriptEngines.h +++ b/libraries/script-engine/src/ScriptEngines.h @@ -86,7 +86,7 @@ protected: void onScriptEngineLoaded(const QString& scriptFilename); void onScriptEngineError(const QString& scriptFilename); void launchScriptEngine(ScriptEngine* engine); - bool isStopped() const; + bool isStopped() const { return _isStopped; } QReadWriteLock _scriptEnginesHashLock; QHash _scriptEnginesHash;