Skip to content

Commit 7d30972

Browse files
akien-mgaakien-mga
committed
Merge pull request godotengine#99539 from RandomShaper/fix_dotnet_rl_deadlock
Avoid deadlocks in multi-threaded management of the C# script map
2 parents e255f60 + f8f5505 commit 7d30972

File tree

2 files changed

+103
-51
lines changed

2 files changed

+103
-51
lines changed

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

Lines changed: 101 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -421,16 +421,29 @@ internal static unsafe godot_bool AddScriptBridge(IntPtr scriptPtr, godot_string
421421

422422
private static unsafe bool AddScriptBridgeCore(IntPtr scriptPtr, string scriptPath)
423423
{
424-
lock (_scriptTypeBiMap.ReadWriteLock)
424+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
425+
try
425426
{
426427
if (!_scriptTypeBiMap.IsScriptRegistered(scriptPtr))
427428
{
428429
if (!_pathTypeBiMap.TryGetScriptType(scriptPath, out Type? scriptType))
429430
return false;
430431

431-
_scriptTypeBiMap.Add(scriptPtr, scriptType);
432+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
433+
try
434+
{
435+
_scriptTypeBiMap.Add(scriptPtr, scriptType);
436+
}
437+
finally
438+
{
439+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
440+
}
432441
}
433442
}
443+
finally
444+
{
445+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
446+
}
434447

435448
return true;
436449
}
@@ -453,7 +466,8 @@ internal static unsafe void GetOrCreateScriptBridgeForPath(godot_string* scriptP
453466

454467
private static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript)
455468
{
456-
lock (_scriptTypeBiMap.ReadWriteLock)
469+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
470+
try
457471
{
458472
if (_scriptTypeBiMap.TryGetScriptPtr(scriptType, out IntPtr scriptPtr))
459473
{
@@ -465,14 +479,19 @@ private static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot
465479
// This path is slower, but it's only executed for the first instantiation of the type
466480
CreateScriptBridgeForType(scriptType, outScript);
467481
}
482+
finally
483+
{
484+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
485+
}
468486
}
469487

470488
internal static unsafe void GetOrLoadOrCreateScriptForType(Type scriptType, godot_ref* outScript)
471489
{
472490
static bool GetPathOtherwiseGetOrCreateScript(Type scriptType, godot_ref* outScript,
473491
[MaybeNullWhen(false)] out string scriptPath)
474492
{
475-
lock (_scriptTypeBiMap.ReadWriteLock)
493+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
494+
try
476495
{
477496
if (_scriptTypeBiMap.TryGetScriptPtr(scriptType, out IntPtr scriptPtr))
478497
{
@@ -500,6 +519,10 @@ static bool GetPathOtherwiseGetOrCreateScript(Type scriptType, godot_ref* outScr
500519
scriptPath = null;
501520
return false;
502521
}
522+
finally
523+
{
524+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
525+
}
503526
}
504527

505528
static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string scriptPath)
@@ -529,7 +552,16 @@ static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string
529552
// IMPORTANT: The virtual path must be added to _pathTypeBiMap before the first
530553
// load of the script, otherwise the loaded script won't be added to _scriptTypeBiMap.
531554
scriptPath = GetVirtualConstructedGenericTypeScriptPath(scriptType, scriptPath);
532-
_pathTypeBiMap.Add(scriptPath, scriptType);
555+
556+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
557+
try
558+
{
559+
_pathTypeBiMap.Add(scriptPath, scriptType);
560+
}
561+
finally
562+
{
563+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
564+
}
533565
}
534566

535567
// This must be done outside the read-write lock, as the script resource loading can lock it
@@ -559,89 +591,108 @@ private static unsafe void CreateScriptBridgeForType(Type scriptType, godot_ref*
559591
{
560592
Debug.Assert(!scriptType.IsGenericTypeDefinition, $"Script type must be a constructed generic type or not generic at all. Type: {scriptType}.");
561593

562-
NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
563-
IntPtr scriptPtr = outScript->Reference;
594+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
595+
try
596+
{
597+
NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
598+
IntPtr scriptPtr = outScript->Reference;
564599

565-
// Caller takes care of locking
566-
_scriptTypeBiMap.Add(scriptPtr, scriptType);
600+
_scriptTypeBiMap.Add(scriptPtr, scriptType);
601+
}
602+
finally
603+
{
604+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
605+
}
567606

568-
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
607+
NativeFuncs.godotsharp_internal_reload_registered_script(outScript->Reference);
569608
}
570609

571610
[UnmanagedCallersOnly]
572611
internal static void RemoveScriptBridge(IntPtr scriptPtr)
573612
{
613+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
574614
try
575615
{
576-
lock (_scriptTypeBiMap.ReadWriteLock)
577-
{
578-
_scriptTypeBiMap.Remove(scriptPtr);
579-
}
616+
_scriptTypeBiMap.Remove(scriptPtr);
580617
}
581618
catch (Exception e)
582619
{
583620
ExceptionUtils.LogException(e);
584621
}
622+
finally
623+
{
624+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
625+
}
585626
}
586627

587628
[UnmanagedCallersOnly]
588629
internal static godot_bool TryReloadRegisteredScriptWithClass(IntPtr scriptPtr)
589630
{
631+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
590632
try
591633
{
592-
lock (_scriptTypeBiMap.ReadWriteLock)
634+
if (_scriptTypeBiMap.TryGetScriptType(scriptPtr, out _))
593635
{
594-
if (_scriptTypeBiMap.TryGetScriptType(scriptPtr, out _))
595-
{
596-
// NOTE:
597-
// Currently, we reload all scripts, not only the ones from the unloaded ALC.
598-
// As such, we need to handle this case instead of treating it as an error.
599-
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
600-
return godot_bool.True;
601-
}
636+
// NOTE:
637+
// Currently, we reload all scripts, not only the ones from the unloaded ALC.
638+
// As such, we need to handle this case instead of treating it as an error.
639+
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
640+
return godot_bool.True;
641+
}
602642

603-
if (!_scriptDataForReload.TryGetValue(scriptPtr, out var dataForReload))
604-
{
605-
GD.PushError("Missing class qualified name for reloading script");
606-
return godot_bool.False;
607-
}
643+
if (!_scriptDataForReload.TryGetValue(scriptPtr, out var dataForReload))
644+
{
645+
GD.PushError("Missing class qualified name for reloading script");
646+
return godot_bool.False;
647+
}
608648

609-
_ = _scriptDataForReload.TryRemove(scriptPtr, out _);
649+
_ = _scriptDataForReload.TryRemove(scriptPtr, out _);
610650

611-
if (dataForReload.assemblyName == null)
612-
{
613-
GD.PushError(
614-
$"Missing assembly name of class '{dataForReload.classFullName}' for reloading script");
615-
return godot_bool.False;
616-
}
651+
if (dataForReload.assemblyName == null)
652+
{
653+
GD.PushError(
654+
$"Missing assembly name of class '{dataForReload.classFullName}' for reloading script");
655+
return godot_bool.False;
656+
}
617657

618-
var scriptType = ReflectionUtils.FindTypeInLoadedAssemblies(dataForReload.assemblyName,
619-
dataForReload.classFullName);
658+
var scriptType = ReflectionUtils.FindTypeInLoadedAssemblies(dataForReload.assemblyName,
659+
dataForReload.classFullName);
620660

621-
if (scriptType == null)
622-
{
623-
// The class was removed, can't reload
624-
return godot_bool.False;
625-
}
661+
if (scriptType == null)
662+
{
663+
// The class was removed, can't reload
664+
return godot_bool.False;
665+
}
626666

627-
if (!typeof(GodotObject).IsAssignableFrom(scriptType))
628-
{
629-
// The class no longer inherits GodotObject, can't reload
630-
return godot_bool.False;
631-
}
667+
if (!typeof(GodotObject).IsAssignableFrom(scriptType))
668+
{
669+
// The class no longer inherits GodotObject, can't reload
670+
return godot_bool.False;
671+
}
632672

673+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
674+
try
675+
{
633676
_scriptTypeBiMap.Add(scriptPtr, scriptType);
677+
}
678+
finally
679+
{
680+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
681+
}
634682

635-
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
683+
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
636684

637-
return godot_bool.True;
638-
}
685+
return godot_bool.True;
639686
}
640687
catch (Exception e)
641688
{
642689
ExceptionUtils.LogException(e);
643690
return godot_bool.False;
644691
}
692+
finally
693+
{
694+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
695+
}
645696
}
646697

647698
private static unsafe void GetScriptTypeInfo(Type scriptType, godot_csharp_type_info* outTypeInfo)

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Diagnostics.CodeAnalysis;
55
using System.Linq;
66
using System.Runtime.CompilerServices;
7+
using System.Threading;
78

89
namespace Godot.Bridge;
910

@@ -13,7 +14,7 @@ public static partial class ScriptManagerBridge
1314
{
1415
private class ScriptTypeBiMap
1516
{
16-
public readonly object ReadWriteLock = new();
17+
public readonly ReaderWriterLockSlim ReadWriteLock = new(LockRecursionPolicy.SupportsRecursion);
1718
private System.Collections.Generic.Dictionary<IntPtr, Type> _scriptTypeMap = new();
1819
private System.Collections.Generic.Dictionary<Type, IntPtr> _typeScriptMap = new();
1920

0 commit comments

Comments
 (0)