Skip to content

Commit f8f5505

Browse files
RandomShaperRandomShaper
committed
Avoid deadlocks in multi-threaded management of the C# script map
1 parent 9e60984 commit f8f5505

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
@@ -433,16 +433,29 @@ internal static unsafe godot_bool AddScriptBridge(IntPtr scriptPtr, godot_string
433433

434434
private static unsafe bool AddScriptBridgeCore(IntPtr scriptPtr, string scriptPath)
435435
{
436-
lock (_scriptTypeBiMap.ReadWriteLock)
436+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
437+
try
437438
{
438439
if (!_scriptTypeBiMap.IsScriptRegistered(scriptPtr))
439440
{
440441
if (!_pathTypeBiMap.TryGetScriptType(scriptPath, out Type? scriptType))
441442
return false;
442443

443-
_scriptTypeBiMap.Add(scriptPtr, scriptType);
444+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
445+
try
446+
{
447+
_scriptTypeBiMap.Add(scriptPtr, scriptType);
448+
}
449+
finally
450+
{
451+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
452+
}
444453
}
445454
}
455+
finally
456+
{
457+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
458+
}
446459

447460
return true;
448461
}
@@ -465,7 +478,8 @@ internal static unsafe void GetOrCreateScriptBridgeForPath(godot_string* scriptP
465478

466479
private static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript)
467480
{
468-
lock (_scriptTypeBiMap.ReadWriteLock)
481+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
482+
try
469483
{
470484
if (_scriptTypeBiMap.TryGetScriptPtr(scriptType, out IntPtr scriptPtr))
471485
{
@@ -477,14 +491,19 @@ private static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot
477491
// This path is slower, but it's only executed for the first instantiation of the type
478492
CreateScriptBridgeForType(scriptType, outScript);
479493
}
494+
finally
495+
{
496+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
497+
}
480498
}
481499

482500
internal static unsafe void GetOrLoadOrCreateScriptForType(Type scriptType, godot_ref* outScript)
483501
{
484502
static bool GetPathOtherwiseGetOrCreateScript(Type scriptType, godot_ref* outScript,
485503
[MaybeNullWhen(false)] out string scriptPath)
486504
{
487-
lock (_scriptTypeBiMap.ReadWriteLock)
505+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
506+
try
488507
{
489508
if (_scriptTypeBiMap.TryGetScriptPtr(scriptType, out IntPtr scriptPtr))
490509
{
@@ -512,6 +531,10 @@ static bool GetPathOtherwiseGetOrCreateScript(Type scriptType, godot_ref* outScr
512531
scriptPath = null;
513532
return false;
514533
}
534+
finally
535+
{
536+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
537+
}
515538
}
516539

517540
static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string scriptPath)
@@ -541,7 +564,16 @@ static string GetVirtualConstructedGenericTypeScriptPath(Type scriptType, string
541564
// IMPORTANT: The virtual path must be added to _pathTypeBiMap before the first
542565
// load of the script, otherwise the loaded script won't be added to _scriptTypeBiMap.
543566
scriptPath = GetVirtualConstructedGenericTypeScriptPath(scriptType, scriptPath);
544-
_pathTypeBiMap.Add(scriptPath, scriptType);
567+
568+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
569+
try
570+
{
571+
_pathTypeBiMap.Add(scriptPath, scriptType);
572+
}
573+
finally
574+
{
575+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
576+
}
545577
}
546578

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

574-
NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
575-
IntPtr scriptPtr = outScript->Reference;
606+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
607+
try
608+
{
609+
NativeFuncs.godotsharp_internal_new_csharp_script(outScript);
610+
IntPtr scriptPtr = outScript->Reference;
576611

577-
// Caller takes care of locking
578-
_scriptTypeBiMap.Add(scriptPtr, scriptType);
612+
_scriptTypeBiMap.Add(scriptPtr, scriptType);
613+
}
614+
finally
615+
{
616+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
617+
}
579618

580-
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
619+
NativeFuncs.godotsharp_internal_reload_registered_script(outScript->Reference);
581620
}
582621

583622
[UnmanagedCallersOnly]
584623
internal static void RemoveScriptBridge(IntPtr scriptPtr)
585624
{
625+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
586626
try
587627
{
588-
lock (_scriptTypeBiMap.ReadWriteLock)
589-
{
590-
_scriptTypeBiMap.Remove(scriptPtr);
591-
}
628+
_scriptTypeBiMap.Remove(scriptPtr);
592629
}
593630
catch (Exception e)
594631
{
595632
ExceptionUtils.LogException(e);
596633
}
634+
finally
635+
{
636+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
637+
}
597638
}
598639

599640
[UnmanagedCallersOnly]
600641
internal static godot_bool TryReloadRegisteredScriptWithClass(IntPtr scriptPtr)
601642
{
643+
_scriptTypeBiMap.ReadWriteLock.EnterUpgradeableReadLock();
602644
try
603645
{
604-
lock (_scriptTypeBiMap.ReadWriteLock)
646+
if (_scriptTypeBiMap.TryGetScriptType(scriptPtr, out _))
605647
{
606-
if (_scriptTypeBiMap.TryGetScriptType(scriptPtr, out _))
607-
{
608-
// NOTE:
609-
// Currently, we reload all scripts, not only the ones from the unloaded ALC.
610-
// As such, we need to handle this case instead of treating it as an error.
611-
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
612-
return godot_bool.True;
613-
}
648+
// NOTE:
649+
// Currently, we reload all scripts, not only the ones from the unloaded ALC.
650+
// As such, we need to handle this case instead of treating it as an error.
651+
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
652+
return godot_bool.True;
653+
}
614654

615-
if (!_scriptDataForReload.TryGetValue(scriptPtr, out var dataForReload))
616-
{
617-
GD.PushError("Missing class qualified name for reloading script");
618-
return godot_bool.False;
619-
}
655+
if (!_scriptDataForReload.TryGetValue(scriptPtr, out var dataForReload))
656+
{
657+
GD.PushError("Missing class qualified name for reloading script");
658+
return godot_bool.False;
659+
}
620660

621-
_ = _scriptDataForReload.TryRemove(scriptPtr, out _);
661+
_ = _scriptDataForReload.TryRemove(scriptPtr, out _);
622662

623-
if (dataForReload.assemblyName == null)
624-
{
625-
GD.PushError(
626-
$"Missing assembly name of class '{dataForReload.classFullName}' for reloading script");
627-
return godot_bool.False;
628-
}
663+
if (dataForReload.assemblyName == null)
664+
{
665+
GD.PushError(
666+
$"Missing assembly name of class '{dataForReload.classFullName}' for reloading script");
667+
return godot_bool.False;
668+
}
629669

630-
var scriptType = ReflectionUtils.FindTypeInLoadedAssemblies(dataForReload.assemblyName,
631-
dataForReload.classFullName);
670+
var scriptType = ReflectionUtils.FindTypeInLoadedAssemblies(dataForReload.assemblyName,
671+
dataForReload.classFullName);
632672

633-
if (scriptType == null)
634-
{
635-
// The class was removed, can't reload
636-
return godot_bool.False;
637-
}
673+
if (scriptType == null)
674+
{
675+
// The class was removed, can't reload
676+
return godot_bool.False;
677+
}
638678

639-
if (!typeof(GodotObject).IsAssignableFrom(scriptType))
640-
{
641-
// The class no longer inherits GodotObject, can't reload
642-
return godot_bool.False;
643-
}
679+
if (!typeof(GodotObject).IsAssignableFrom(scriptType))
680+
{
681+
// The class no longer inherits GodotObject, can't reload
682+
return godot_bool.False;
683+
}
644684

685+
_scriptTypeBiMap.ReadWriteLock.EnterWriteLock();
686+
try
687+
{
645688
_scriptTypeBiMap.Add(scriptPtr, scriptType);
689+
}
690+
finally
691+
{
692+
_scriptTypeBiMap.ReadWriteLock.ExitWriteLock();
693+
}
646694

647-
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
695+
NativeFuncs.godotsharp_internal_reload_registered_script(scriptPtr);
648696

649-
return godot_bool.True;
650-
}
697+
return godot_bool.True;
651698
}
652699
catch (Exception e)
653700
{
654701
ExceptionUtils.LogException(e);
655702
return godot_bool.False;
656703
}
704+
finally
705+
{
706+
_scriptTypeBiMap.ReadWriteLock.ExitUpgradeableReadLock();
707+
}
657708
}
658709

659710
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)