From 4091557388051e98eb6d4d2059875480e85f5311 Mon Sep 17 00:00:00 2001
From: ksuprynowicz <ksuprynowicz@post.pl>
Date: Tue, 1 Aug 2023 16:33:41 +0200
Subject: [PATCH] Fixed deadlock when entity script engines are being reset.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Julian Groß julian.g@posteo.de
---
 .../src/EntityTreeRenderer.cpp                | 37 +++++++++++++++----
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp
index a6d8646af6..e667f6cf9d 100644
--- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp
+++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp
@@ -42,6 +42,7 @@
 #include "RenderableWebEntityItem.h"
 
 #include <PointerManager.h>
+#include <QtConcurrent/QtConcurrentRun>
 
 std::function<bool()> EntityTreeRenderer::_entitiesShouldFadeFunction = []() { return true; };
 
@@ -217,11 +218,15 @@ void EntityTreeRenderer::setupEntityScriptEngineSignals(const ScriptManagerPoint
 }
 
 void EntityTreeRenderer::resetPersistentEntitiesScriptEngine() {
+    // This runs script engine shutdown procedure in a separate thread, avoiding a deadlock when script engine is doing
+    // a blocking call to main thread
     if (_persistentEntitiesScriptManager) {
-        _persistentEntitiesScriptManager->unloadAllEntityScripts(true);
-        _persistentEntitiesScriptManager->stop();
-        _persistentEntitiesScriptManager->waitTillDoneRunning();
-        _persistentEntitiesScriptManager->disconnectNonEssentialSignals();
+        QtConcurrent::run([manager = _persistentEntitiesScriptManager] {
+            manager->unloadAllEntityScripts(true);
+            manager->stop();
+            manager->waitTillDoneRunning();
+            manager->disconnectNonEssentialSignals();
+        });
     }
     _persistentEntitiesScriptManager = scriptManagerFactory(ScriptManager::ENTITY_CLIENT_SCRIPT, NO_SCRIPT,
                                                 QString("about:Entities %1").arg(++_entitiesScriptEngineCount));
@@ -235,11 +240,15 @@ void EntityTreeRenderer::resetPersistentEntitiesScriptEngine() {
 }
 
 void EntityTreeRenderer::resetNonPersistentEntitiesScriptEngine() {
+    // This runs script engine shutdown procedure in a separate thread, avoiding a deadlock when script engine is doing
+    // a blocking call to main thread
     if (_nonPersistentEntitiesScriptManager) {
-        _nonPersistentEntitiesScriptManager->unloadAllEntityScripts(true);
-        _nonPersistentEntitiesScriptManager->stop();
-        _nonPersistentEntitiesScriptManager->waitTillDoneRunning();
-        _nonPersistentEntitiesScriptManager->disconnectNonEssentialSignals();
+        QtConcurrent::run([manager = _nonPersistentEntitiesScriptManager] {
+            manager->unloadAllEntityScripts(true);
+            manager->stop();
+            manager->waitTillDoneRunning();
+            manager->disconnectNonEssentialSignals();
+        });
     }
     _nonPersistentEntitiesScriptManager = scriptManagerFactory(ScriptManager::ENTITY_CLIENT_SCRIPT, NO_SCRIPT,
                                                 QString("about:Entities %1").arg(++_entitiesScriptEngineCount));
@@ -311,11 +320,23 @@ void EntityTreeRenderer::clear() {
         // unload and stop the engines
         if (_nonPersistentEntitiesScriptManager) {
             // do this here (instead of in deleter) to avoid marshalling unload signals back to this thread
+
+            // TODO: blocking call will cause deadlocks if the script engine is doing blocking call to main thread,
+            //  for example to access resource cache.
+            //  Since there's no event loop running at this time anymore, I have no easy workaround for this.
+            //  This could be solved by replacing all calls to quit() with calls to a new function that will do
+            //  a cleanup first while event loop is still running
             _nonPersistentEntitiesScriptManager->unloadAllEntityScripts(true);
             _nonPersistentEntitiesScriptManager->stop();
         }
         if (_persistentEntitiesScriptManager) {
             // do this here (instead of in deleter) to avoid marshalling unload signals back to this thread
+
+            // TODO: blocking call will cause deadlocks if the script engine is doing blocking call to main thread,
+            //  for example to access resource cache.
+            //  Since there's no event loop running at this time anymore, I have no easy workaround for this.
+            //  This could be solved by replacing all calls to quit() with calls to a new function that will do
+            //  a cleanup first while event loop is still running
             _persistentEntitiesScriptManager->unloadAllEntityScripts(true);
             _persistentEntitiesScriptManager->stop();
         }