Merge pull request #7853 from zzmp/fix/shutdown

Fix hang on exit from dying scripts

interface/src/Application.cpp

@@ -1156,10 +1156,16 @@ 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<ScriptEngines>()->saveScripts();
     DependencyManager::get<ScriptEngines>()->shutdownScripting(); // stop all currently running global scripts
     DependencyManager::destroy<ScriptEngines>();
 
+    // 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();

interface/src/ui/overlays/Overlays.cpp

@@ -36,15 +36,8 @@
 #include <QtQuick/QQuickWindow>
 
 
-Overlays::Overlays() : _nextOverlayID(1) {
-    connect(qApp, &Application::beforeAboutToQuit, [=] {
-        cleanupAllOverlays();
-    });
-}
-
-Overlays::~Overlays() {
-}
-
+Overlays::Overlays() :
+    _nextOverlayID(1) {}
 
 void Overlays::cleanupAllOverlays() {
     {

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<unsigned int, Overlay::Pointer> _overlaysHUD;
     QMap<unsigned int, Overlay::Pointer> _overlaysWorld;

libraries/script-engine/src/ScriptEngine.cpp

@@ -62,8 +62,6 @@
 
 #include "MIDIEvent.h"
 
-std::atomic<bool> ScriptEngine::_stoppingAllScripts { false };
-
 static const QString SCRIPT_EXCEPTION_FORMAT = "[UncaughtException] %1 in %2:%3";
 
 Q_DECLARE_METATYPE(QScriptEngine::FunctionSignature)
@@ -756,7 +754,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<ScriptEngines>()->isStopped()) {
         return QScriptValue(); // bail early
     }
 
@@ -792,7 +790,7 @@ QScriptValue ScriptEngine::evaluate(const QString& sourceCode, const QString& fi
 }
 
 void ScriptEngine::run() {
-    if (_stoppingAllScripts) {
+    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
         return; // bail early - avoid setting state in init(), as evaluate() will bail too
     }
 
@@ -1025,7 +1023,7 @@ QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int
 }
 
 QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS) {
-    if (_stoppingAllScripts) {
+    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
         qCDebug(scriptengine) << "Script.setInterval() while shutting down is ignored... parent script:" << getFilename();
         return NULL; // bail early
     }
@@ -1034,7 +1032,7 @@ QObject* ScriptEngine::setInterval(const QScriptValue& function, int intervalMS)
 }
 
 QObject* ScriptEngine::setTimeout(const QScriptValue& function, int timeoutMS) {
-    if (_stoppingAllScripts) {
+    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
         qCDebug(scriptengine) << "Script.setTimeout() while shutting down is ignored... parent script:" << getFilename();
         return NULL; // bail early
     }
@@ -1086,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 (_stoppingAllScripts) {
+    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
         qCDebug(scriptengine) << "Script.include() while shutting down is ignored..."
         << "includeFiles:" << includeFiles << "parent script:" << getFilename();
         return; // bail early
@@ -1184,7 +1182,7 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac
 }
 
 void ScriptEngine::include(const QString& includeFile, QScriptValue callback) {
-    if (_stoppingAllScripts) {
+    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
         qCDebug(scriptengine) << "Script.include() while shutting down is ignored... "
             << "includeFile:" << includeFile << "parent script:" << getFilename();
         return; // bail early
@@ -1199,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 (_stoppingAllScripts) {
+    if (DependencyManager::get<ScriptEngines>()->isStopped()) {
         qCDebug(scriptengine) << "Script.load() while shutting down is ignored... "
             << "loadFile:" << loadFile << "parent script:" << getFilename();
         return; // bail early

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();
@@ -232,9 +234,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<void()> operation);
     void callWithEnvironment(const EntityItemID& entityID, const QUrl& sandboxURL, QScriptValue function, QScriptValue thisObject, QScriptValueList args);
-
-    friend class ScriptEngines;
-    static std::atomic<bool> _stoppingAllScripts;
 };
 
 #endif // hifi_ScriptEngine_h

libraries/script-engine/src/ScriptEngines.cpp

@@ -119,26 +119,27 @@ void ScriptEngines::registerScriptInitializer(ScriptInitializer initializer) {
 }
 
 void ScriptEngines::addScriptEngine(ScriptEngine* engine) {
-    _allScriptsMutex.lock();
-    _allKnownScriptEngines.insert(engine);
-    _allScriptsMutex.unlock();
+    if (_isStopped) {
+        engine->deleteLater();
+    } else {
+        QMutexLocker locker(&_allScriptsMutex);
+        _allKnownScriptEngines.insert(engine);
+    }
 }
 
 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) {
-        _allScriptsMutex.lock();
+    if (!_isStopped) {
+        QMutexLocker locker(&_allScriptsMutex);
         _allKnownScriptEngines.remove(engine);
-        _allScriptsMutex.unlock();
     }
 }
 
 void ScriptEngines::shutdownScripting() {
-    _allScriptsMutex.lock();
-    _stoppingAllScripts = true;
-    ScriptEngine::_stoppingAllScripts = true;
+    _isStopped = true;
+    QMutexLocker locker(&_allScriptsMutex);
     qCDebug(scriptengine) << "Stopping all scripts.... currently known scripts:" << _allKnownScriptEngines.size();
 
     QMutableSetIterator<ScriptEngine*> i(_allKnownScriptEngines);
@@ -174,8 +175,6 @@ void ScriptEngines::shutdownScripting() {
             i.remove();
         }
     }
-    _stoppingAllScripts = false;
-    _allScriptsMutex.unlock();
     qCDebug(scriptengine) << "DONE Stopping all scripts....";
 }
 
@@ -499,7 +498,6 @@ void ScriptEngines::launchScriptEngine(ScriptEngine* scriptEngine) {
     }
 }
 
-
 void ScriptEngines::onScriptFinished(const QString& rawScriptURL, ScriptEngine* engine) {
     bool removed = false;
     {

libraries/script-engine/src/ScriptEngines.h

@@ -86,16 +86,17 @@ protected:
     void onScriptEngineLoaded(const QString& scriptFilename);
     void onScriptEngineError(const QString& scriptFilename);
     void launchScriptEngine(ScriptEngine* engine);
+    bool isStopped() const { return _isStopped; }
 
     QReadWriteLock _scriptEnginesHashLock;
     QHash<QUrl, ScriptEngine*> _scriptEnginesHash;
     QSet<ScriptEngine*> _allKnownScriptEngines;
     QMutex _allScriptsMutex;
-    std::atomic<bool> _stoppingAllScripts { false };
     std::list<ScriptInitializer> _scriptInitializers;
     mutable Setting::Handle<QString> _scriptsLocationHandle;
     ScriptsModel _scriptsModel;
     ScriptsModelFilter _scriptsModelFilter;
+    std::atomic<bool> _isStopped { false };
 };
 
 QUrl normalizeScriptURL(const QUrl& rawScriptURL);