C#: Possible deadlock during GD.Load() if the garbage collector disposes a script while the loader tries to register one

[bs-mwoerner]

Tested versions

v4.2.1.stable.mono.official [b09f793]

System information

Godot v4.2.1.stable.mono - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2070 (NVIDIA; 31.0.15.4633) - Intel(R) Core(TM) i7-10875H CPU @ 2.30GHz (16 Threads)

Issue description

There's probably no good way to reproduce this reliably, so I don't know how much value this report has, but there appears to be a possible deadlock in how C# scripts are created and disposed.

When loading a scene via GD.Load<PackedScene>(), there seems to be a chance that the .NET Garbage Collector running on a separate thread disposes a resource containing a script while the loading code on the main thread tries to register a new script for the new scene. With a bit of unlucky timing, this can lead to the garbage collector waiting for the main thread to release the _scriptTypeBiMap.ReadWriteLock lock while the main thread is waiting for the garbage collector to release the CSharpLanguage::get_singleton()->script_instances_mutex mutex, which deadlocks the game.

Here's how this might look like:

Main Thread
I can't look into the C++ code from here, but it's presumably waiting on the CSharpLanguage::get_singleton()->script_instances_mutex mutex at

godot/modules/mono/csharp_script.cpp

Lines 2816 to 2825 in b09f793

	CSharpScript::CSharpScript() {
	_clear();
	#ifdef DEBUG_ENABLED
	{
	MutexLock lock(CSharpLanguage::get_singleton()->script_instances_mutex);
	CSharpLanguage::get_singleton()->script_list.add(&this->script_list);
	}
	#endif
	}

after having taken the _scriptTypeBiMap.ReadWriteLock lock at

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

Lines 454 to 468 in b09f793

	private static unsafe void GetOrCreateScriptBridgeForType(Type scriptType, godot_ref* outScript)
	{
	lock (_scriptTypeBiMap.ReadWriteLock)
	{
	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);
	}
	}

[External Code] (Unknown Source:0)
GodotSharp.dll!Godot.NativeInterop.NativeFuncs.godotsharp_internal_new_csharp_script(Godot.NativeInterop.godot_ref* r_dest) Line 162 (Godot.NativeInterop.NativeFuncs.generated.cs:162)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.CreateScriptBridgeForType(System.Type scriptType, Godot.NativeInterop.godot_ref* outScript) Line 515 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:515)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForType(System.Type scriptType, Godot.NativeInterop.godot_ref* outScript) Line 466 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:466)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForPath(Godot.NativeInterop.godot_string* scriptPath, Godot.NativeInterop.godot_ref* outScript) Line 451 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:451)
[External Code] (Unknown Source:0)
GodotSharp.dll!Godot.NativeInterop.NativeFuncs.godotsharp_internal_script_load(Godot.NativeInterop.godot_string p_path, Godot.NativeInterop.godot_ref* r_dest) Line 169 (Godot.NativeInterop.NativeFuncs.generated.cs:169)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.GetOrLoadOrCreateScriptForType(System.Type scriptType, Godot.NativeInterop.godot_ref* outScript) Line 502 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:502)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.UpdateScriptClassInfo(System.IntPtr scriptPtr, Godot.NativeInterop.godot_string* outClassName, Godot.NativeInterop.godot_bool* outTool, Godot.NativeInterop.godot_bool* outGlobal, Godot.NativeInterop.godot_bool* outAbstract, Godot.NativeInterop.godot_string* outIconPath, Godot.NativeInterop.godot_array* outMethodsDest, Godot.NativeInterop.godot_dictionary* outRpcFunctionsDest, Godot.NativeInterop.godot_dictionary* outEventSignalsDest, Godot.NativeInterop.godot_ref* outBaseScript) Line 793 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:793)
[External Code] (Unknown Source:0)
GodotSharp.dll!Godot.NativeInterop.NativeFuncs.godotsharp_internal_reload_registered_script(System.IntPtr scriptPtr) Line 175 (Godot.NativeInterop.NativeFuncs.generated.cs:175)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.CreateScriptBridgeForType(System.Type scriptType, Godot.NativeInterop.godot_ref* outScript) Line 521 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:521)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForType(System.Type scriptType, Godot.NativeInterop.godot_ref* outScript) Line 466 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:466)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.GetOrCreateScriptBridgeForPath(Godot.NativeInterop.godot_string* scriptPath, Godot.NativeInterop.godot_ref* outScript) Line 451 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:451)
[External Code] (Unknown Source:0)
GodotSharp.dll!Godot.NativeInterop.NativeFuncs.godotsharp_method_bind_ptrcall(System.IntPtr p_method_bind, System.IntPtr p_instance, void** p_args, void* p_ret) Line 353 (Godot.NativeInterop.NativeFuncs.generated.cs:353)
GodotSharp.dll!Godot.NativeCalls.godot_icall_3_981(System.IntPtr method, System.IntPtr ptr, string arg1, string arg2, int arg3) Line 9004 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Generated\NativeCalls.cs:9004)
GodotSharp.dll!Godot.ResourceLoader.Load(string path, string typeHint, Godot.ResourceLoader.CacheMode cacheMode) Line 100 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Generated\GodotObjects\ResourceLoader.cs:100)
GodotSharp.dll!Godot.ResourceLoader.Load<Godot.PackedScene>(string path, string typeHint, Godot.ResourceLoader.CacheMode cacheMode) Line 27 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Extensions\ResourceLoaderExtensions.cs:27)
GodotSharp.dll!Godot.GD.Load<Godot.PackedScene>(string path) Line 129 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\GD.cs:129)

GC Finalizer Thread

Waiting on the _scriptTypeBiMap.ReadWriteLock at

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

Lines 524 to 538 in b09f793

	[UnmanagedCallersOnly]
	internal static void RemoveScriptBridge(IntPtr scriptPtr)
	{
	try
	{
	lock (_scriptTypeBiMap.ReadWriteLock)
	{
	_scriptTypeBiMap.Remove(scriptPtr);
	}
	}
	catch (Exception e)
	{
	ExceptionUtils.LogException(e);
	}
	}

after presumably having taken the CSharpLanguage::get_singleton()->script_instances_mutex mutex at

godot/modules/mono/csharp_script.cpp

Lines 2827 to 2836 in b09f793

	CSharpScript::~CSharpScript() {
	#ifdef DEBUG_ENABLED
	MutexLock lock(CSharpLanguage::get_singleton()->script_instances_mutex);
	CSharpLanguage::get_singleton()->script_list.remove(&this->script_list);
	#endif
	if (GDMonoCache::godot_api_cache_updated) {
	GDMonoCache::managed_callbacks.ScriptManagerBridge_RemoveScriptBridge(this);
	}
	}

[Waiting on lock owned by Thread 7844] (Unknown Source:0)
[External Code] (Unknown Source:0)
GodotSharp.dll!Godot.Bridge.ScriptManagerBridge.RemoveScriptBridge(System.IntPtr scriptPtr) Line 529 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\ScriptManagerBridge.cs:529)
[External Code] (Unknown Source:0)
GodotSharp.dll!Godot.NativeInterop.NativeFuncs.godotsharp_internal_refcounted_disposed(System.IntPtr ptr, System.IntPtr gcHandleToFree, Godot.NativeInterop.godot_bool isFinalizer) Line 109 (Godot.NativeInterop.NativeFuncs.generated.cs:109)
GodotSharp.dll!Godot.GodotObject.Dispose(bool disposing) Line 117 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\GodotObject.base.cs:117)
GodotSharp.dll!Godot.GodotObject.~GodotObject() Line 80 (\root\godot\modules\mono\glue\GodotSharp\GodotSharp\Core\GodotObject.base.cs:80)
[External Code] (Unknown Source:0)

Notes

From the #ifdefs it seems that this only affects DEBUG builds. There apparently is a way to disable background garbage collection in .NET, although performance-wise, it may be preferable to find a way to make script creation and disposal thread-safe. At first glance, it may not be necessary for ~CSharpScript() to hold on to the mutex while calling ScriptManagerBridge_RemoveScriptBridge().

Steps to reproduce

None, unfortunately. Requires a project that uses C# scripts, certain circumstances that trigger a garbage collection during a GD.Load(), and possibly some unlucky timing. Hopefully, the problem can be diagnosed based on the code and the stack traces.

Minimal reproduction project (MRP)

N/A

[AlexOtsuka]

Is it maybe possible to reproduce this bug more consistently by calling GC.Collect() in various spots in different threads to attempt forcing garbage collection to run during GD.Load()?

[AlexTheRegent]

It looks like I'm experiencing exactly this issue in my project. I'm making city building game, and once game is loaded (with dynamic chunks and objects), it permanently freezes game 80-90% of the time if I try to open building's menu (music is playing in background, but window is completely non-responsive. no errors are printed in console even with --verbose editor flag). It also randomly freezes during loading of menus during playing (with extremely low chance, like once per week). I found this issue by searching for GD.Load keyword, because when I removed this line of code, I couldn't reproduce this issue. After reading description of this issue, I tested release build, and it does not freezes for the same save slot I'm consistently reproducing this issue from editor. Also if I open different game menu before opening building or try to open menu during world loading (I spawn 8 objects per frame), freeze does not occur either. So I think this issue is real and probably not so rare, but good thing is that seems like it does not affect release builds.

[bs-mwoerner]

Yes, that sounds very similar. For me, the deadlock is very rare in general but very consistent when loading a specific savefile.

By the way, for my case, I was able to work around this for the moment by doing GC.Collect() right before GD.Load().

[AlexTheRegent]

Thank you for sharing workaround! For anyone who would stumble upon this issue I would also suggest to wrap GC.Collect() by #if TOOLS defines set to avoid manual GC calls in release builds:

#if TOOLS
GC.Collect()
#endif