From f8f5505b263dca2951007b2013a5c8ee647bab6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= Date: Fri, 29 Nov 2024 10:10:49 +0100 Subject: [PATCH] Avoid deadlocks in multi-threaded management of the C# script map --- .../Core/Bridge/ScriptManagerBridge.cs | 165 ++++++++++++------ .../Core/Bridge/ScriptManagerBridge.types.cs | 3 +- 2 files changed, 110 insertions(+), 58 deletions(-) diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs index 91d49854c73..287a273609b 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs @@ -433,16 +433,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; } @@ -465,7 +478,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)) { @@ -477,6 +491,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) @@ -484,7 +502,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)) { @@ -512,6 +531,10 @@ namespace Godot.Bridge scriptPath = null; return false; } + finally + { + _scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock(); + } } static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string scriptPath) @@ -541,7 +564,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 @@ -571,89 +603,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; - } - - if (!_scriptDataForReload.TryGetValue(scriptPtr, out var dataForReload)) - { - GD.PushError("Missing class qualified name for reloading script"); - return godot_bool.False; - } - - _ = _scriptDataForReload.TryRemove(scriptPtr, out _); - - 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); - - 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; - } - - _scriptTypeBiMap.Add(scriptPtr, scriptType); - + // 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; + } + + _ = _scriptDataForReload.TryRemove(scriptPtr, out _); + + 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); + + 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; + } + + _scriptTypeBiMap.ReadWriteLock.EnterWriteLock(); + try + { + _scriptTypeBiMap.Add(scriptPtr, scriptType); + } + finally + { + _scriptTypeBiMap.ReadWriteLock.ExitWriteLock(); + } + + NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr); + + 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) diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs index 29fa13d6259..aca9595ecba 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs +++ b/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 _scriptTypeMap = new(); private System.Collections.Generic.Dictionary _typeScriptMap = new();