Merge pull request #99539 from RandomShaper/fix_dotnet_rl_deadlock

Avoid deadlocks in multi-threaded management of the C# script map

modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs

@@ -421,16 +421,29 @@ namespace Godot.Bridge
 
         private static unsafe bool AddScriptBridgeCore(IntPtr scriptPtr, string scriptPath)
         {
-            lock (_scriptTypeBiMap.ReadWriteLock)
+            _scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
+            try
             {
                 if (!_scriptTypeBiMap.IsScriptRegistered(scriptPtr))
                 {
                     if (!_pathTypeBiMap.TryGetScriptType(scriptPath, out Type? scriptType))
                         return false;
 
-                    _scriptTypeBiMap.Add(scriptPtr, scriptType);
+                    _scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
+                    try
+                    {
+                        _scriptTypeBiMap.Add(scriptPtr, scriptType);
+                    }
+                    finally
+                    {
+                        _scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
+                    }
                 }
             }
+            finally
+            {
+                _scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
+            }
 
             return true;
         }
@@ -453,7 +466,8 @@ namespace Godot.Bridge
 
         private static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript)
         {
-            lock (_scriptTypeBiMap.ReadWriteLock)
+            _scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
+            try
             {
                 if (_scriptTypeBiMap.TryGetScriptPtr(scriptType, out IntPtr scriptPtr))
                 {
@@ -465,6 +479,10 @@ namespace Godot.Bridge
                 // This path is slower, but it's only executed for the first instantiation of the type
                 CreateScriptBridgeForType(scriptType, outScript);
             }
+            finally
+            {
+                _scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
+            }
         }
 
         internal static unsafe void GetOrLoadOrCreateScriptForType(Type scriptType, godot_ref* outScript)
@@ -472,7 +490,8 @@ namespace Godot.Bridge
             static bool GetPathOtherwiseGetOrCreateScript(Type scriptType, godot_ref* outScript,
                 [MaybeNullWhen(false)] out string scriptPath)
             {
-                lock (_scriptTypeBiMap.ReadWriteLock)
+                _scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
+                try
                 {
                     if (_scriptTypeBiMap.TryGetScriptPtr(scriptType, out IntPtr scriptPtr))
                     {
@@ -500,6 +519,10 @@ namespace Godot.Bridge
                     scriptPath = null;
                     return false;
                 }
+                finally
+                {
+                    _scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
+                }
             }
 
             static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string scriptPath)
@@ -529,7 +552,16 @@ namespace Godot.Bridge
                     // IMPORTANT: The virtual path must be added to _pathTypeBiMap before the first
                     // load of the script, otherwise the loaded script won't be added to _scriptTypeBiMap.
                     scriptPath = GetVirtualConstructedGenericTypeScriptPath(scriptType, scriptPath);
-                    _pathTypeBiMap.Add(scriptPath, scriptType);
+
+                    _scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
+                    try
+                    {
+                        _pathTypeBiMap.Add(scriptPath, scriptType);
+                    }
+                    finally
+                    {
+                        _scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
+                    }
                 }
 
                 // This must be done outside the read-write lock, as the script resource loading can lock it
@@ -559,89 +591,108 @@ namespace Godot.Bridge
         {
             Debug.Assert(!scriptType.IsGenericTypeDefinition, $"Script type must be a constructed generic type or not generic at all. Type: {scriptType}.");
 
-            NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
-            IntPtr scriptPtr = outScript->Reference;
+            _scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
+            try
+            {
+                NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
+                IntPtr scriptPtr = outScript->Reference;
 
-            // Caller takes care of locking
-            _scriptTypeBiMap.Add(scriptPtr, scriptType);
+                _scriptTypeBiMap.Add(scriptPtr, scriptType);
+            }
+            finally
+            {
+                _scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
+            }
 
-            NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
+            NativeFuncs.godotsharp_internal_reload_registered_script(outScript->Reference);
         }
 
         [UnmanagedCallersOnly]
         internal static void RemoveScriptBridge(IntPtr scriptPtr)
         {
+            _scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
             try
             {
-                lock (_scriptTypeBiMap.ReadWriteLock)
-                {
-                    _scriptTypeBiMap.Remove(scriptPtr);
-                }
+                _scriptTypeBiMap.Remove(scriptPtr);
             }
             catch (Exception e)
             {
                 ExceptionUtils.LogException(e);
             }
+            finally
+            {
+                _scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
+            }
         }
 
         [UnmanagedCallersOnly]
         internal static godot_bool TryReloadRegisteredScriptWithClass(IntPtr scriptPtr)
         {
+            _scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
             try
             {
-                lock (_scriptTypeBiMap.ReadWriteLock)
+                if (_scriptTypeBiMap.TryGetScriptType(scriptPtr, out _))
                 {
-                    if (_scriptTypeBiMap.TryGetScriptType(scriptPtr, out _))
-                    {
-                        // NOTE:
-                        // Currently, we reload all scripts, not only the ones from the unloaded ALC.
-                        // As such, we need to handle this case instead of treating it as an error.
-                        NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
-                        return godot_bool.True;
-                    }
+                    // NOTE:
+                    // Currently, we reload all scripts, not only the ones from the unloaded ALC.
+                    // As such, we need to handle this case instead of treating it as an error.
+                    NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
+                    return godot_bool.True;
+                }
 
-                    if (!_scriptDataForReload.TryGetValue(scriptPtr, out var dataForReload))
-                    {
-                        GD.PushError("Missing class qualified name for reloading script");
-                        return godot_bool.False;
-                    }
+                if (!_scriptDataForReload.TryGetValue(scriptPtr, out var dataForReload))
+                {
+                    GD.PushError("Missing class qualified name for reloading script");
+                    return godot_bool.False;
+                }
 
-                    _ = _scriptDataForReload.TryRemove(scriptPtr, out _);
+                _ = _scriptDataForReload.TryRemove(scriptPtr, out _);
 
-                    if (dataForReload.assemblyName == null)
-                    {
-                        GD.PushError(
-                            $"Missing assembly name of class '{dataForReload.classFullName}' for reloading script");
-                        return godot_bool.False;
-                    }
+                if (dataForReload.assemblyName == null)
+                {
+                    GD.PushError(
+                        $"Missing assembly name of class '{dataForReload.classFullName}' for reloading script");
+                    return godot_bool.False;
+                }
 
-                    var scriptType = ReflectionUtils.FindTypeInLoadedAssemblies(dataForReload.assemblyName,
-                        dataForReload.classFullName);
+                var scriptType = ReflectionUtils.FindTypeInLoadedAssemblies(dataForReload.assemblyName,
+                    dataForReload.classFullName);
 
-                    if (scriptType == null)
-                    {
-                        // The class was removed, can't reload
-                        return godot_bool.False;
-                    }
+                if (scriptType == null)
+                {
+                    // The class was removed, can't reload
+                    return godot_bool.False;
+                }
 
-                    if (!typeof(GodotObject).IsAssignableFrom(scriptType))
-                    {
-                        // The class no longer inherits GodotObject, can't reload
-                        return godot_bool.False;
-                    }
+                if (!typeof(GodotObject).IsAssignableFrom(scriptType))
+                {
+                    // The class no longer inherits GodotObject, can't reload
+                    return godot_bool.False;
+                }
 
+                _scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
+                try
+                {
                     _scriptTypeBiMap.Add(scriptPtr, scriptType);
+                }
+                finally
+                {
+                    _scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
+                }
 
-                    NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
+                NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
 
-                    return godot_bool.True;
-                }
+                return godot_bool.True;
             }
             catch (Exception e)
             {
                 ExceptionUtils.LogException(e);
                 return godot_bool.False;
             }
+            finally
+            {
+                _scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
+            }
         }
 
         private static unsafe void GetScriptTypeInfo(Type scriptType, godot_csharp_type_info* outTypeInfo)

modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs

@@ -4,6 +4,7 @@ using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Runtime.CompilerServices;
+using System.Threading;
 
 namespace Godot.Bridge;
 
@@ -13,7 +14,7 @@ public static partial class ScriptManagerBridge
 {
     private class ScriptTypeBiMap
     {
-        public readonly object ReadWriteLock = new();
+        public readonly ReaderWriterLockSlim ReadWriteLock = new(LockRecursionPolicy.SupportsRecursion);
         private System.Collections.Generic.Dictionary<IntPtr, Type> _scriptTypeMap = new();
         private System.Collections.Generic.Dictionary<Type, IntPtr> _typeScriptMap = new();