From ce46c3064754f940cf3ba6a10609e956a57f55b6 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Mon, 23 Nov 2015 13:09:02 -0800 Subject: [PATCH 1/2] change the ScriptEngine::waitTillDoneRunning() to wait for the script thread to complete --- .../src/EntityTreeRenderer.cpp | 2 +- libraries/script-engine/src/ScriptEngine.cpp | 28 ++++++++++--------- libraries/script-engine/src/ScriptEngine.h | 20 ++++++------- 3 files changed, 26 insertions(+), 24 deletions(-) diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 8c81ceaeb8..8775040feb 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -122,7 +122,7 @@ void EntityTreeRenderer::init() { } void EntityTreeRenderer::shutdown() { - _entitiesScriptEngine->disconnect(); // disconnect all slots/signals from the script engine + _entitiesScriptEngine->disconnectNonEssentialSignals(); // disconnect all slots/signals from the script engine, except essential _shuttingDown = true; } diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index f313cd7d9e..0fdc5169b6 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -124,14 +124,9 @@ static bool hadUncaughtExceptions(QScriptEngine& engine, const QString& fileName ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNameString, bool wantSignals) : _scriptContents(scriptContents), - _isFinished(false), - _isRunning(false), - _isInitialized(false), _timerFunctionMap(), _wantSignals(wantSignals), _fileNameString(fileNameString), - _isUserLoaded(false), - _isReloading(false), _arrayBufferClass(new ArrayBufferClass(this)) { _allScriptsMutex.lock(); @@ -140,6 +135,8 @@ ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNam } ScriptEngine::~ScriptEngine() { + qDebug() << "Script Engine shutting down (destructor) for script:" << getFilename(); + // 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 @@ -150,7 +147,13 @@ ScriptEngine::~ScriptEngine() { } } +void ScriptEngine::disconnectNonEssentialSignals() { + disconnect(); + connect(this, &ScriptEngine::doneRunning, thread(), &QThread::quit); +} + void ScriptEngine::runInThread() { + _isThreaded = true; QThread* workerThread = new QThread(); // thread is not owned, so we need to manage the delete QString scriptEngineName = QString("Script Thread:") + getFilename(); workerThread->setObjectName(scriptEngineName); @@ -176,12 +179,13 @@ void ScriptEngine::runInThread() { QSet ScriptEngine::_allKnownScriptEngines; QMutex ScriptEngine::_allScriptsMutex; bool ScriptEngine::_stoppingAllScripts = false; -bool ScriptEngine::_doneRunningThisScript = false; void ScriptEngine::stopAllScripts(QObject* application) { _allScriptsMutex.lock(); _stoppingAllScripts = true; + qDebug() << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size(); + QMutableSetIterator i(_allKnownScriptEngines); while (i.hasNext()) { ScriptEngine* scriptEngine = i.next(); @@ -219,7 +223,9 @@ void ScriptEngine::stopAllScripts(QObject* application) { // We need to wait for the engine to be done running before we proceed, because we don't // want any of the scripts final "scriptEnding()" or pending "update()" methods from accessing // any application state after we leave this stopAllScripts() method + qDebug() << "waiting on script:" << scriptName; scriptEngine->waitTillDoneRunning(); + qDebug() << "done waiting on script:" << scriptName; // If the script is stopped, we can remove it from our set i.remove(); @@ -227,21 +233,19 @@ void ScriptEngine::stopAllScripts(QObject* application) { } _stoppingAllScripts = false; _allScriptsMutex.unlock(); + qDebug() << "DONE Stopping all scripts...."; } void ScriptEngine::waitTillDoneRunning() { // If the script never started running or finished running before we got here, we don't need to wait for it - if (_isRunning) { - - _doneRunningThisScript = false; // NOTE: this is static, we serialize our waiting for scripts to finish + if (_isRunning && _isThreaded) { // NOTE: waitTillDoneRunning() will be called on the main Application thread, inside of stopAllScripts() // we want the application thread to continue to process events, because the scripts will likely need to // marshall messages across to the main thread. For example if they access Settings or Meny in any of their // shutdown code. - while (!_doneRunningThisScript) { - + while (thread()->isRunning()) { // process events for the main application thread, allowing invokeMethod calls to pass between threads QCoreApplication::processEvents(); } @@ -752,8 +756,6 @@ void ScriptEngine::run() { emit runningStateChanged(); emit doneRunning(); } - - _doneRunningThisScript = true; } // NOTE: This is private because it must be called on the same thread that created the timers, which is why diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 1412ba7aaf..0cf9eb2c10 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -128,6 +128,7 @@ public: bool isFinished() const { return _isFinished; } // used by Application and ScriptWidget bool isRunning() const { return _isRunning; } // used by ScriptWidget + void disconnectNonEssentialSignals(); static void stopAllScripts(QObject* application); // used by Application on shutdown //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -165,15 +166,16 @@ signals: protected: QString _scriptContents; QString _parentURL; - bool _isFinished; - bool _isRunning; - int _evaluatesPending = 0; - bool _isInitialized; + bool _isFinished { false }; + bool _isRunning { false }; + int _evaluatesPending { 0 }; + bool _isInitialized { false }; QHash _timerFunctionMap; QSet _includedURLs; - bool _wantSignals = true; + bool _wantSignals { true }; QHash _entityScripts; -private: + bool _isThreaded { false }; + void init(); QString getFilename() const; void waitTillDoneRunning(); @@ -191,8 +193,8 @@ private: Quat _quatLibrary; Vec3 _vec3Library; ScriptUUID _uuidLibrary; - bool _isUserLoaded; - bool _isReloading; + bool _isUserLoaded { false }; + bool _isReloading { false }; ArrayBufferClass* _arrayBufferClass; @@ -205,8 +207,6 @@ private: static QSet _allKnownScriptEngines; static QMutex _allScriptsMutex; static bool _stoppingAllScripts; - static bool _doneRunningThisScript; - }; #endif // hifi_ScriptEngine_h From 5f88d958ab592f21122eaa388363700bb0052fe4 Mon Sep 17 00:00:00 2001 From: Brad Hefta-Gaub Date: Mon, 23 Nov 2015 18:26:15 -0800 Subject: [PATCH 2/2] CR feedback --- libraries/script-engine/src/ScriptEngine.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 0fdc5169b6..995a92bf83 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -135,7 +135,7 @@ ScriptEngine::ScriptEngine(const QString& scriptContents, const QString& fileNam } ScriptEngine::~ScriptEngine() { - qDebug() << "Script Engine shutting down (destructor) for script:" << getFilename(); + qCDebug(scriptengine) << "Script Engine shutting down (destructor) for script:" << getFilename(); // 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 @@ -158,6 +158,10 @@ void ScriptEngine::runInThread() { QString scriptEngineName = QString("Script Thread:") + getFilename(); workerThread->setObjectName(scriptEngineName); + // NOTE: If you connect any essential signals for proper shutdown or cleanup of + // the script engine, make sure to add code to "reconnect" them to the + // disconnectNonEssentialSignals() method + // when the worker thread is started, call our engine's run.. connect(workerThread, &QThread::started, this, &ScriptEngine::run); @@ -184,7 +188,7 @@ void ScriptEngine::stopAllScripts(QObject* application) { _allScriptsMutex.lock(); _stoppingAllScripts = true; - qDebug() << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size(); + qCDebug(scriptengine) << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size(); QMutableSetIterator i(_allKnownScriptEngines); while (i.hasNext()) { @@ -223,9 +227,9 @@ void ScriptEngine::stopAllScripts(QObject* application) { // We need to wait for the engine to be done running before we proceed, because we don't // want any of the scripts final "scriptEnding()" or pending "update()" methods from accessing // any application state after we leave this stopAllScripts() method - qDebug() << "waiting on script:" << scriptName; + qCDebug(scriptengine) << "waiting on script:" << scriptName; scriptEngine->waitTillDoneRunning(); - qDebug() << "done waiting on script:" << scriptName; + qCDebug(scriptengine) << "done waiting on script:" << scriptName; // If the script is stopped, we can remove it from our set i.remove(); @@ -233,7 +237,7 @@ void ScriptEngine::stopAllScripts(QObject* application) { } _stoppingAllScripts = false; _allScriptsMutex.unlock(); - qDebug() << "DONE Stopping all scripts...."; + qCDebug(scriptengine) << "DONE Stopping all scripts...."; } @@ -1170,7 +1174,7 @@ void ScriptEngine::refreshFileScript(const EntityItemID& entityID) { QString filePath = QUrl(details.scriptText).toLocalFile(); auto lastModified = QFileInfo(filePath).lastModified().toMSecsSinceEpoch(); if (lastModified > details.lastModified) { - qDebug() << "Reloading modified script " << details.scriptText; + qCDebug(scriptengine) << "Reloading modified script " << details.scriptText; QFile file(filePath); file.open(QIODevice::ReadOnly);