Merge pull request #6473 from ZappoMan/waitForScriptThreads

change the ScriptEngine::waitTillDoneRunning() to wait for the script thread to complete

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;
 }
 

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() {
+    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
     // because that method removes scripts from its list as it iterates them
@@ -150,11 +147,21 @@ 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);
 
+    // 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);
 
@@ -176,12 +183,13 @@ void ScriptEngine::runInThread() {
 QSet<ScriptEngine*> ScriptEngine::_allKnownScriptEngines;
 QMutex ScriptEngine::_allScriptsMutex;
 bool ScriptEngine::_stoppingAllScripts = false;
-bool ScriptEngine::_doneRunningThisScript = false;
 
 void ScriptEngine::stopAllScripts(QObject* application) {
     _allScriptsMutex.lock();
     _stoppingAllScripts = true;
 
+    qCDebug(scriptengine) << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size();
+
     QMutableSetIterator<ScriptEngine*> i(_allKnownScriptEngines);
     while (i.hasNext()) {
         ScriptEngine* scriptEngine = i.next();
@@ -219,7 +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
+            qCDebug(scriptengine) << "waiting on script:" << scriptName;
             scriptEngine->waitTillDoneRunning();
+            qCDebug(scriptengine) << "done waiting on script:" << scriptName;
 
             // If the script is stopped, we can remove it from our set
             i.remove();
@@ -227,21 +237,19 @@ void ScriptEngine::stopAllScripts(QObject* application) {
     }
     _stoppingAllScripts = false;
     _allScriptsMutex.unlock();
+    qCDebug(scriptengine) << "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 +760,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
@@ -1168,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);

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<QTimer*, QScriptValue> _timerFunctionMap;
     QSet<QUrl> _includedURLs;
-    bool _wantSignals = true;
+    bool _wantSignals { true };
     QHash<EntityItemID, EntityScriptDetails> _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<ScriptEngine*> _allKnownScriptEngines;
     static QMutex _allScriptsMutex;
     static bool _stoppingAllScripts;
-    static bool _doneRunningThisScript;
-
 };
 
 #endif // hifi_ScriptEngine_h