Merge pull request #853 from overte-org/fix/script_deadlocks

Move helper script engines to their own threads

assignment-client/src/avatars/ScriptableAvatar.cpp

@@ -30,7 +30,7 @@
 #include <NetworkingConstants.h>
 
 
-ScriptableAvatar::ScriptableAvatar(): _scriptEngine(newScriptEngine()) {
+ScriptableAvatar::ScriptableAvatar() {
     _clientTraitsHandler.reset(new ClientTraitsHandler(this));
     static std::once_flag once;
     std::call_once(once, [] {
@@ -344,7 +344,9 @@ AvatarEntityMap ScriptableAvatar::getAvatarEntityDataInternal(bool allProperties
             EntityItemProperties properties = entity->getProperties(desiredProperties);
 
             QByteArray blob;
-            EntityItemProperties::propertiesToBlob(*_scriptEngine, sessionID, properties, blob, allProperties);
+            _helperScriptEngine.run( [&] {
+                EntityItemProperties::propertiesToBlob(*_helperScriptEngine.get(), sessionID, properties, blob, allProperties);
+            });
             data[id] = blob;
         }
     });
@@ -368,8 +370,12 @@ void ScriptableAvatar::setAvatarEntityData(const AvatarEntityMap& avatarEntityDa
     while (dataItr != avatarEntityData.end()) {
         EntityItemProperties properties;
         const QByteArray& blob = dataItr.value();
-        if (!blob.isNull() && EntityItemProperties::blobToProperties(*_scriptEngine, blob, properties)) {
+        if (!blob.isNull()) {
-            newProperties[dataItr.key()] = properties;
+            _helperScriptEngine.run([&] {
+                if (EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), blob, properties)) {
+                    newProperties[dataItr.key()] = properties;
+                }
+            });
         }
         ++dataItr;
     }
@@ -448,9 +454,16 @@ void ScriptableAvatar::updateAvatarEntity(const QUuid& entityID, const QByteArra
 
     EntityItemPointer entity;
     EntityItemProperties properties;
-    if (!EntityItemProperties::blobToProperties(*_scriptEngine, entityData, properties)) {
+    {
-        // entityData is corrupt
+        // TODO: checking how often this happens and what is the performance impact of having the script engine on separate thread
-        return;
+        // If it's happening often, a method to move HelperScriptEngine into the current thread would be a good idea
+        bool result = _helperScriptEngine.runWithResult<bool> ( [&]() {
+            return EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), entityData, properties);
+        });
+        if (!result) {
+            // entityData is corrupt
+            return;
+        }
     }
 
     std::map<QUuid, EntityItemPointer>::iterator itr = _entities.find(entityID);

assignment-client/src/avatars/ScriptableAvatar.h

@@ -21,6 +21,7 @@
 #include <EntityItem.h>
 #include "model-networking/ModelCache.h"
 #include "Rig.h"
+#include <HelperScriptEngine.h>
 
 /*@jsdoc
  * The <code>Avatar</code> API is used to manipulate scriptable avatars on the domain. This API is a subset of the 
@@ -228,7 +229,7 @@ private:
     QHash<QString, int> _fstJointIndices; ///< 1-based, since zero is returned for missing keys
     QStringList _fstJointNames; ///< in order of depth-first traversal
     QUrl _skeletonModelFilenameURL; // This contains URL from filename field in fst file
-    mutable ScriptEnginePointer _scriptEngine;
+    mutable HelperScriptEngine _helperScriptEngine;
     std::map<QUuid, EntityItemPointer> _entities;
 
     /// Loads the joint indices, names from the FST file (if any)

interface/src/avatar/MyAvatar.cpp

@@ -1740,10 +1740,11 @@ void MyAvatar::handleChangedAvatarEntityData() {
                 blobFailed = true; // blob doesn't exist
                 return;
             }
-            std::lock_guard<std::mutex> guard(_scriptEngineLock);
+            _helperScriptEngine.run( [&] {
-            if (!EntityItemProperties::blobToProperties(*_scriptEngine, itr.value(), properties)) {
+                if (!EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), itr.value(), properties)) {
-                blobFailed = true; // blob is corrupt
+                    blobFailed = true;  // blob is corrupt
-            }
+                }
+            });
         });
         if (blobFailed) {
             // remove from _cachedAvatarEntityBlobUpdatesToSkip just in case:
@@ -1776,10 +1777,11 @@ void MyAvatar::handleChangedAvatarEntityData() {
                 skip = true;
                 return;
             }
-            std::lock_guard<std::mutex> guard(_scriptEngineLock);
+            _helperScriptEngine.run( [&] {
-            if (!EntityItemProperties::blobToProperties(*_scriptEngine, itr.value(), properties)) {
+                if (!EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), itr.value(), properties)) {
-                skip = true;
+                    skip = true;
-            }
+                }
+            });
         });
         if (!skip && canRezAvatarEntites) {
             sanitizeAvatarEntityProperties(properties);
@@ -1884,10 +1886,9 @@ bool MyAvatar::updateStaleAvatarEntityBlobs() const {
         if (found) {
             ++numFound;
             QByteArray blob;
-            {
+            _helperScriptEngine.run( [&] {
-                std::lock_guard<std::mutex> guard(_scriptEngineLock);
+                EntityItemProperties::propertiesToBlob(*_helperScriptEngine.get(), getID(), properties, blob);
-                EntityItemProperties::propertiesToBlob(*_scriptEngine, getID(), properties, blob);
+            });
-            }
             _avatarEntitiesLock.withWriteLock([&] {
                 _cachedAvatarEntityBlobs[id] = blob;
             });
@@ -1948,10 +1949,9 @@ AvatarEntityMap MyAvatar::getAvatarEntityData() const {
         EntityItemProperties properties = entity->getProperties(desiredProperties);
 
         QByteArray blob;
-        {
+        _helperScriptEngine.run( [&] {
-            std::lock_guard<std::mutex> guard(_scriptEngineLock);
+            EntityItemProperties::propertiesToBlob(*_helperScriptEngine.get(), getID(), properties, blob, true);
-            EntityItemProperties::propertiesToBlob(*_scriptEngine, getID(), properties, blob, true);
+        });
-        }
 
         data[entityID] = blob;
     }
@@ -2093,9 +2093,6 @@ void MyAvatar::avatarEntityDataToJson(QJsonObject& root) const {
 }
 
 void MyAvatar::loadData() {
-    if (!_scriptEngine) {
-        _scriptEngine = newScriptEngine();
-    }
     getHead()->setBasePitch(_headPitchSetting.get());
 
     _yawSpeed = _yawSpeedSetting.get(_yawSpeed);
@@ -2704,11 +2701,10 @@ QVariantList MyAvatar::getAvatarEntitiesVariant() {
             QVariantMap avatarEntityData;
             avatarEntityData["id"] = entityID;
             EntityItemProperties entityProperties = entity->getProperties(desiredProperties);
-            {
+            _helperScriptEngine.run( [&] {
-                std::lock_guard<std::mutex> guard(_scriptEngineLock);
+                ScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_helperScriptEngine.get(), entityProperties);
-                ScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_scriptEngine.get(), entityProperties);
                 avatarEntityData["properties"] = scriptProperties.toVariant();
-            }
+            });
             avatarEntitiesData.append(QVariant(avatarEntityData));
         }
     }

interface/src/avatar/MyAvatar.h

@@ -28,6 +28,7 @@
 #include <controllers/Pose.h>
 #include <controllers/Actions.h>
 #include <EntityItem.h>
+#include <HelperScriptEngine.h>
 #include <ThreadSafeValueCache.h>
 #include <Rig.h>
 #include <SettingHandle.h>
@@ -3102,8 +3103,10 @@ private:
     mutable std::set<EntityItemID> _staleCachedAvatarEntityBlobs;
     //
     // keep a ScriptEngine around so we don't have to instantiate on the fly (these are very slow to create/delete)
-    mutable std::mutex _scriptEngineLock;
+    // TODO: profile if it performs better when script engine is on avatar thread or on its own thread
-    ScriptEnginePointer _scriptEngine { nullptr };
+    // Own thread is safer from deadlocks
+    mutable HelperScriptEngine _helperScriptEngine;
+
     bool _needToSaveAvatarEntitySettings { false };
 
     bool _reactionTriggers[NUM_AVATAR_TRIGGER_REACTIONS] { false, false };

libraries/baking/src/MaterialBaker.cpp

@@ -36,8 +36,7 @@ MaterialBaker::MaterialBaker(const QString& materialData, bool isURL, const QStr
     _isURL(isURL),
     _destinationPath(destinationPath),
     _bakedOutputDir(bakedOutputDir),
-    _textureOutputDir(bakedOutputDir + "/materialTextures/" + QString::number(materialNum++)),
+    _textureOutputDir(bakedOutputDir + "/materialTextures/" + QString::number(materialNum++))
-    _scriptEngine(newScriptEngine())
 {
 }
 
@@ -214,16 +213,20 @@ void MaterialBaker::outputMaterial() {
         if (_materialResource->parsedMaterials.networkMaterials.size() == 1) {
             auto networkMaterial = _materialResource->parsedMaterials.networkMaterials.begin();
             auto scriptableMaterial = scriptable::ScriptableMaterial(networkMaterial->second);
-            QVariant materialVariant =
+            _helperScriptEngine.run( [&] {
-                scriptable::scriptableMaterialToScriptValue(_scriptEngine.get(), scriptableMaterial).toVariant();
+                QVariant materialVariant =
-            json.insert("materials", QJsonDocument::fromVariant(materialVariant).object());
+                    scriptable::scriptableMaterialToScriptValue(_helperScriptEngine.get(), scriptableMaterial).toVariant();
+                json.insert("materials", QJsonDocument::fromVariant(materialVariant).object());
+            });
         } else {
             QJsonArray materialArray;
             for (auto networkMaterial : _materialResource->parsedMaterials.networkMaterials) {
                 auto scriptableMaterial = scriptable::ScriptableMaterial(networkMaterial.second);
-                QVariant materialVariant =
+                _helperScriptEngine.run( [&] {
-                    scriptable::scriptableMaterialToScriptValue(_scriptEngine.get(), scriptableMaterial).toVariant();
+                    QVariant materialVariant =
-                materialArray.append(QJsonDocument::fromVariant(materialVariant).object());
+                        scriptable::scriptableMaterialToScriptValue(_helperScriptEngine.get(), scriptableMaterial).toVariant();
+                    materialArray.append(QJsonDocument::fromVariant(materialVariant).object());
+                });
             }
             json.insert("materials", materialArray);
         }

libraries/baking/src/MaterialBaker.h

@@ -21,6 +21,7 @@
 #include "TextureBaker.h"
 #include "baking/TextureFileNamer.h"
 
+#include <HelperScriptEngine.h>
 #include <procedural/ProceduralMaterialCache.h>
 #include <ScriptEngine.h>
 
@@ -72,7 +73,7 @@ private:
     QString _textureOutputDir;
     QString _bakedMaterialData;
 
-    ScriptEnginePointer _scriptEngine;
+    HelperScriptEngine _helperScriptEngine;
     static std::function<QThread*()> _getNextOvenWorkerThreadOperator;
     TextureFileNamer _textureFileNamer;
 

libraries/entities/src/EntityTree.cpp

@@ -2553,11 +2553,10 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer
     }
     entityDescription["DataVersion"] = _persistDataVersion;
     entityDescription["Id"] = _persistID;
-    const std::lock_guard<std::mutex> scriptLock(scriptEngineMutex);
+    _helperScriptEngine.run( [&] {
-    RecurseOctreeToMapOperator theOperator(entityDescription, element, scriptEngine.get(), skipDefaultValues,
+        RecurseOctreeToMapOperator theOperator(entityDescription, element, _helperScriptEngine.get(), skipDefaultValues,
-                                            skipThoseWithBadParents, _myAvatar);
+                                               skipThoseWithBadParents, _myAvatar);
-    withReadLock([&] {
+        withReadLock([&] { recurseTreeWithOperator(&theOperator); });
-        recurseTreeWithOperator(&theOperator);
     });
     return true;
 }
@@ -2728,11 +2727,10 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) {
         }
 
         EntityItemProperties properties;
-        {
+        _helperScriptEngine.run( [&] {
-            const std::lock_guard<std::mutex> scriptLock(scriptEngineMutex);
+            ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *_helperScriptEngine.get());
-            ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *scriptEngine);
             EntityItemPropertiesFromScriptValueIgnoreReadOnly(entityScriptValue, properties);
-        }
+        });
 
         EntityItemID entityItemID;
         if (entityMap.contains("id")) {
@@ -2881,13 +2879,12 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) {
 }
 
 bool EntityTree::writeToJSON(QString& jsonString, const OctreeElementPointer& element) {
-    const std::lock_guard<std::mutex> scriptLock(scriptEngineMutex);
+    _helperScriptEngine.run( [&] {
-    RecurseOctreeToJSONOperator theOperator(element, scriptEngine.get(), jsonString);
+        RecurseOctreeToJSONOperator theOperator(element, _helperScriptEngine.get(), jsonString);
-    withReadLock([&] {
+        withReadLock([&] { recurseTreeWithOperator(&theOperator); });
-        recurseTreeWithOperator(&theOperator);
-    });
 
-    jsonString = theOperator.getJson();
+        jsonString = theOperator.getJson();
+    });
     return true;
 }
 

libraries/entities/src/EntityTree.h

@@ -15,6 +15,7 @@
 #include <QSet>
 #include <QVector>
 
+#include <HelperScriptEngine.h>
 #include <Octree.h>
 #include <SpatialParentFinder.h>
 
@@ -387,8 +388,7 @@ private:
                                          MovingEntitiesOperator& moveOperator, bool force, bool tellServer);
 
     // Script engine for writing entity tree data to and from JSON
-    std::mutex scriptEngineMutex;
+    HelperScriptEngine _helperScriptEngine;
-    ScriptEnginePointer scriptEngine{ newScriptEngine() };
 };
 
 void convertGrabUserDataToProperties(EntityItemProperties& properties);

libraries/script-engine/src/HelperScriptEngine.cpp

@@ -0,0 +1,31 @@
+//
+//  HelperScriptEngine.h
+//  libraries/script-engine/src/HelperScriptEngine.h
+//
+//  Created by dr Karol Suprynowicz on 2024/04/28.
+//  Copyright 2024 Overte e.V.
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+#include "HelperScriptEngine.h"
+
+HelperScriptEngine::HelperScriptEngine() {
+    std::lock_guard<std::mutex> lock(_scriptEngineLock);
+    _scriptEngine = newScriptEngine();
+    _scriptEngineThread.reset(new QThread());
+    _scriptEngine->setThread(_scriptEngineThread.get());
+    _scriptEngineThread->start();
+}
+
+HelperScriptEngine::~HelperScriptEngine() {
+    std::lock_guard<std::mutex> lock(_scriptEngineLock);
+    if (_scriptEngine) {
+        if (_scriptEngineThread) {
+            _scriptEngineThread->quit();
+            _scriptEngineThread->wait();
+        }
+        _scriptEngine.reset();
+    }
+}

libraries/script-engine/src/HelperScriptEngine.h

@@ -0,0 +1,65 @@
+//
+//  HelperScriptEngine.h
+//  libraries/script-engine/src/HelperScriptEngine.h
+//
+//  Created by dr Karol Suprynowicz on 2024/04/28.
+//  Copyright 2024 Overte e.V.
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//
+
+#ifndef overte_HelperScriptEngine_h
+#define overte_HelperScriptEngine_h
+
+#include <mutex>
+#include "QThread"
+
+#include "ScriptEngine.h"
+
+/**
+ * @brief Provides a wrapper around script engine that does not have ScriptManager
+ *
+ * HelperScriptEngine is used for performing smaller tasks, like for example conversions between entity
+ * properties and JSON data.
+ * For thread safety all accesses to helper script engine need to be done either through HelperScriptEngine::run()
+ * or HelperScriptEngine::runWithResult().
+ *
+ */
+
+
+class HelperScriptEngine {
+public:
+    HelperScriptEngine();
+    ~HelperScriptEngine();
+
+    template <typename F>
+    inline void run(F&& f) {
+        std::lock_guard<std::mutex> guard(_scriptEngineLock);
+        f();
+    }
+
+    template <typename T, typename F>
+    inline T runWithResult(F&& f) {
+        T result;
+        {
+            std::lock_guard<std::mutex> guard(_scriptEngineLock);
+            result = f();
+        }
+        return result;
+    }
+
+    /**
+     * @brief Returns pointer to the script engine
+     *
+     * This function should be used only inside HelperScriptEngine::run() or HelperScriptEngine::runWithResult()
+     */
+    ScriptEngine* get() { return _scriptEngine.get(); };
+    ScriptEnginePointer getShared() { return _scriptEngine; };
+private:
+    std::mutex _scriptEngineLock;
+    ScriptEnginePointer _scriptEngine { nullptr };
+    std::shared_ptr<QThread> _scriptEngineThread { nullptr };
+};
+
+#endif  //overte_HelperScriptEngine_h