Skip to content

Commit

Permalink
Avoid deadlocks in multi-threaded management of the C# script map
Browse files Browse the repository at this point in the history
  • Loading branch information
@RandomShaper
RandomShaper committed Nov 29, 2024
1 parent 9e60984 commit aad25f5
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 56 deletions.
161 changes: 106 additions & 55 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,16 +433,29 @@ internal static unsafe godot_bool AddScriptBridge(IntPtr scriptPtr, godot_string

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;
}
Expand All @@ -455,7 +468,6 @@ internal static unsafe void GetOrCreateScriptBridgeForPath(godot_string* scriptP
if (!_pathTypeBiMap.TryGetScriptType(scriptPathStr, out Type? scriptType))
{
NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
return;
}

Debug.Assert(!scriptType.IsGenericTypeDefinition, $"Cannot get or create script for a generic type definition '{scriptType.FullName}'. Path: '{scriptPathStr}'.");
Expand All @@ -465,17 +477,23 @@ internal static unsafe void GetOrCreateScriptBridgeForPath(godot_string* scriptP

private static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript)
{
lock (_scriptTypeBiMap.ReadWriteLock)
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
try
{
if (_scriptTypeBiMap.TryGetScriptPtr(scriptType, out IntPtr scriptPtr))
{
// Use existing
NativeFuncs.godotsharp_ref_new_from_ref_counted_ptr(out *outScript, scriptPtr);
return;
}

// This path is slower, but it's only executed for the first instantiation of the type
CreateScriptBridgeForType(scriptType, outScript);
else
{
// This path is slower, but it's only executed for the first instantiation of the type
CreateScriptBridgeForType(scriptType, outScript);
}
}
finally
{
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
}
}

Expand All @@ -484,7 +502,8 @@ internal static unsafe void GetOrLoadOrCreateScriptForType(Type scriptType, godo
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))
{
Expand Down Expand Up @@ -512,6 +531,10 @@ static bool GetPathOtherwiseGetOrCreateScript(Type scriptType, godot_ref* outScr
scriptPath = null;
return false;
}
finally
{
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
}
}

static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string scriptPath)
Expand Down Expand Up @@ -541,7 +564,16 @@ static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string
// 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
Expand Down Expand Up @@ -571,89 +603,108 @@ private static unsafe void CreateScriptBridgeForType(Type scriptType, godot_ref*
{
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)
Expand Down
3 changes: 2 additions & 1 deletion modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;

namespace Godot.Bridge;

Expand All @@ -13,7 +14,7 @@ public static partial class ScriptManagerBridge
{
private class ScriptTypeBiMap
{
public readonly object ReadWriteLock = new();
public ReaderWriterLockSlim ReadWriteLock = new(LockRecursionPolicy.SupportsRecursion);
private System.Collections.Generic.Dictionary<IntPtr, Type> _scriptTypeMap = new();
private System.Collections.Generic.Dictionary<Type, IntPtr> _typeScriptMap = new();

Expand Down

0 comments on commit aad25f5

Please sign in to comment.