Merge pull request #75533 from RedworkDE/net-no-reload-noncollectible

C#: Fix editor integration breaking and causing error spam when reloading assemblies fails

doc/classes/ProjectSettings.xml

@@ -818,6 +818,9 @@
 		<member name="dotnet/project/assembly_name" type="String" setter="" getter="" default="&quot;&quot;">
 			Name of the .NET assembly. This name is used as the name of the [code].csproj[/code] and [code].sln[/code] files. By default, it's set to the name of the project ([member application/config/name]) allowing to change it in the future without affecting the .NET assembly.
 		</member>
+		<member name="dotnet/project/assembly_reload_attempts" type="int" setter="" getter="" default="3">
+			Number of times to attempt assembly reloading after rebuilding .NET assembies. Effectively also the timeout in seconds to wait for unloading of script assemblies to finish.
+		</member>
 		<member name="dotnet/project/solution_directory" type="String" setter="" getter="" default="&quot;&quot;">
 			Directory that contains the [code].sln[/code] file. By default, the [code].sln[/code] files is in the root of the project directory, next to the [code]project.godot[/code] and [code].csproj[/code] files.
 			Changing this value allows setting up a multi-project scenario where there are multiple [code].csproj[/code]. Keep in mind that the Godot project is considered one of the C# projects in the workspace and it's root directory should contain the [code]project.godot[/code] and [code].csproj[/code] next to each other.

main/main.cpp

@@ -2756,6 +2756,7 @@ bool Main::start() {
 		// Default values should be synced with mono_gd/gd_mono.cpp.
 		GLOBAL_DEF("dotnet/project/assembly_name", "");
 		GLOBAL_DEF("dotnet/project/solution_directory", "");
+		GLOBAL_DEF(PropertyInfo(Variant::INT, "dotnet/project/assembly_reload_attempts", PROPERTY_HINT_RANGE, "1,16,1,or_greater"), 3);
 #endif
 
 		Error err;

modules/mono/csharp_script.cpp

@@ -120,6 +120,7 @@ void CSharpLanguage::init() {
 	GLOBAL_DEF("dotnet/project/assembly_name", "");
 #ifdef TOOLS_ENABLED
 	GLOBAL_DEF("dotnet/project/solution_directory", "");
+	GLOBAL_DEF(PropertyInfo(Variant::INT, "dotnet/project/assembly_reload_attempts", PROPERTY_HINT_RANGE, "1,16,1,or_greater"), 3);
 #endif
 
 	gdmono = memnew(GDMono);
@@ -770,10 +771,6 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 		return;
 	}
 
-	// TODO:
-	//  Currently, this reloads all scripts, including those whose class is not part of the
-	//  assembly load context being unloaded. As such, we unnecessarily reload GodotTools.
-
 	print_verbose(".NET: Reloading assemblies...");
 
 	// There is no soft reloading with Mono. It's always hard reloading.
@@ -784,8 +781,19 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 		MutexLock lock(script_instances_mutex);
 
 		for (SelfList<CSharpScript> *elem = script_list.first(); elem; elem = elem->next()) {
-			// Cast to CSharpScript to avoid being erased by accident
-			scripts.push_back(Ref<CSharpScript>(elem->self()));
+			bool is_reloadable = false;
+			for (Object *obj : elem->self()->instances) {
+				ERR_CONTINUE(!obj->get_script_instance());
+				CSharpInstance *csi = static_cast<CSharpInstance *>(obj->get_script_instance());
+				if (GDMonoCache::managed_callbacks.GCHandleBridge_GCHandleIsTargetCollectible(csi->get_gchandle_intptr())) {
+					is_reloadable = true;
+					break;
+				}
+			}
+			if (is_reloadable) {
+				// Cast to CSharpScript to avoid being erased by accident.
+				scripts.push_back(Ref<CSharpScript>(elem->self()));
+			}
 		}
 	}
 
@@ -800,6 +808,10 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 
 			ERR_CONTINUE(managed_callable->delegate_handle.value == nullptr);
 
+			if (!GDMonoCache::managed_callbacks.GCHandleBridge_GCHandleIsTargetCollectible(managed_callable->delegate_handle)) {
+				continue;
+			}
+
 			Array serialized_data;
 
 			bool success = GDMonoCache::managed_callbacks.DelegateUtils_TrySerializeDelegateWithGCHandle(
@@ -907,6 +919,15 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) {
 		scr->_clear();
 	}
 
+	// Release the delegates that were serialized earlier.
+	{
+		MutexLock lock(ManagedCallable::instances_mutex);
+
+		for (KeyValue<ManagedCallable *, Array> &kv : ManagedCallable::instances_pending_reload) {
+			kv.key->release_delegate_handle();
+		}
+	}
+
 	// Do domain reload
 	if (gdmono->reload_project_assemblies() != OK) {
 		// Failed to reload the scripts domain
@@ -1158,19 +1179,6 @@ bool CSharpLanguage::debug_break(const String &p_error, bool p_allow_continue) {
 	}
 }
 
-void CSharpLanguage::_on_scripts_domain_about_to_unload() {
-#ifdef GD_MONO_HOT_RELOAD
-	{
-		MutexLock lock(ManagedCallable::instances_mutex);
-
-		for (SelfList<ManagedCallable> *elem = ManagedCallable::instances.first(); elem; elem = elem->next()) {
-			ManagedCallable *managed_callable = elem->self();
-			managed_callable->release_delegate_handle();
-		}
-	}
-#endif
-}
-
 #ifdef TOOLS_ENABLED
 void CSharpLanguage::_editor_init_callback() {
 	// Load GodotTools and initialize GodotSharpEditor

modules/mono/csharp_script.h

@@ -347,7 +347,6 @@ class CSharpLanguage : public ScriptLanguage {
 	String _debug_error;
 
 	friend class GDMono;
-	void _on_scripts_domain_about_to_unload();
 
 #ifdef TOOLS_ENABLED
 	EditorPlugin *godotsharp_editor = nullptr;

modules/mono/glue/GodotSharp/GodotPlugins/Main.cs

@@ -21,6 +21,13 @@ public static class Main
         private sealed class PluginLoadContextWrapper
         {
             private PluginLoadContext? _pluginLoadContext;
+            private readonly WeakReference _weakReference;
+
+            private PluginLoadContextWrapper(PluginLoadContext pluginLoadContext, WeakReference weakReference)
+            {
+                _pluginLoadContext = pluginLoadContext;
+                _weakReference = weakReference;
+            }
 
             public string? AssemblyLoadedPath
             {
@@ -31,7 +38,14 @@ public string? AssemblyLoadedPath
             public bool IsCollectible
             {
                 [MethodImpl(MethodImplOptions.NoInlining)]
-                get => _pluginLoadContext?.IsCollectible ?? false;
+                // if _pluginLoadContext is null we already started unloading, so it was collectible
+                get => _pluginLoadContext?.IsCollectible ?? true;
+            }
+
+            public bool IsAlive
+            {
+                [MethodImpl(MethodImplOptions.NoInlining)]
+                get => _weakReference.IsAlive;
             }
 
             [MethodImpl(MethodImplOptions.NoInlining)]
@@ -43,19 +57,13 @@ public static (Assembly, PluginLoadContextWrapper) CreateAndLoadFromAssemblyName
                 bool isCollectible
             )
             {
-                var wrapper = new PluginLoadContextWrapper();
-                wrapper._pluginLoadContext = new PluginLoadContext(
-                    pluginPath, sharedAssemblies, mainLoadContext, isCollectible);
-                var assembly = wrapper._pluginLoadContext.LoadFromAssemblyName(assemblyName);
+                var context = new PluginLoadContext(pluginPath, sharedAssemblies, mainLoadContext, isCollectible);
+                var reference = new WeakReference(context, trackResurrection: true);
+                var wrapper = new PluginLoadContextWrapper(context, reference);
+                var assembly = context.LoadFromAssemblyName(assemblyName);
                 return (assembly, wrapper);
             }
 
-            [MethodImpl(MethodImplOptions.NoInlining)]
-            public WeakReference CreateWeakReference()
-            {
-                return new WeakReference(_pluginLoadContext, trackResurrection: true);
-            }
-
             [MethodImpl(MethodImplOptions.NoInlining)]
             internal void Unload()
             {
@@ -165,7 +173,7 @@ private static unsafe IntPtr LoadToolsAssembly(char* nAssemblyPath,
                 if (_editorApiAssembly == null)
                     throw new InvalidOperationException("The Godot editor API assembly is not loaded.");
 
-                var (assembly, _) = LoadPlugin(assemblyPath, isCollectible: _editorHint);
+                var (assembly, _) = LoadPlugin(assemblyPath, isCollectible: false);
 
                 NativeLibrary.SetDllImportResolver(assembly, _dllImportResolver!);
 
@@ -236,32 +244,29 @@ private static bool UnloadPlugin(ref PluginLoadContextWrapper? pluginLoadContext
 
                 Console.WriteLine("Unloading assembly load context...");
 
-                var alcWeakReference = pluginLoadContext.CreateWeakReference();
-
                 pluginLoadContext.Unload();
-                pluginLoadContext = null;
 
                 int startTimeMs = Environment.TickCount;
                 bool takingTooLong = false;
 
-                while (alcWeakReference.IsAlive)
+                while (pluginLoadContext.IsAlive)
                 {
                     GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
                     GC.WaitForPendingFinalizers();
 
-                    if (!alcWeakReference.IsAlive)
+                    if (!pluginLoadContext.IsAlive)
                         break;
 
                     int elapsedTimeMs = Environment.TickCount - startTimeMs;
 
-                    if (!takingTooLong && elapsedTimeMs >= 2000)
+                    if (!takingTooLong && elapsedTimeMs >= 200)
                     {
                         takingTooLong = true;
 
                         // TODO: How to log from GodotPlugins? (delegate pointer?)
                         Console.Error.WriteLine("Assembly unloading is taking longer than expected...");
                     }
-                    else if (elapsedTimeMs >= 5000)
+                    else if (elapsedTimeMs >= 1000)
                     {
                         // TODO: How to log from GodotPlugins? (delegate pointer?)
                         Console.Error.WriteLine(
@@ -273,6 +278,7 @@ private static bool UnloadPlugin(ref PluginLoadContextWrapper? pluginLoadContext
 
                 Console.WriteLine("Assembly load context unloaded successfully.");
 
+                pluginLoadContext = null;
                 return true;
             }
             catch (Exception e)

modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/GCHandleBridge.cs

@@ -18,5 +18,26 @@ internal static void FreeGCHandle(IntPtr gcHandlePtr)
                 ExceptionUtils.LogException(e);
             }
         }
+
+        // Returns true, if releasing the provided handle is necessary for assembly unloading to succeed.
+        // This check is not perfect and only intended to prevent things in GodotTools from being reloaded.
+        [UnmanagedCallersOnly]
+        internal static godot_bool GCHandleIsTargetCollectible(IntPtr gcHandlePtr)
+        {
+            try
+            {
+                var target = GCHandle.FromIntPtr(gcHandlePtr).Target;
+
+                if (target is Delegate @delegate)
+                    return DelegateUtils.IsDelegateCollectible(@delegate).ToGodotBool();
+
+                return target.GetType().IsCollectible.ToGodotBool();
+            }
+            catch (Exception e)
+            {
+                ExceptionUtils.LogException(e);
+                return godot_bool.True;
+            }
+        }
     }
 }

modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ManagedCallbacks.cs

@@ -38,6 +38,7 @@ public unsafe struct ManagedCallbacks
         public delegate* unmanaged<IntPtr, godot_dictionary*, godot_dictionary*, void> CSharpInstanceBridge_SerializeState;
         public delegate* unmanaged<IntPtr, godot_dictionary*, godot_dictionary*, void> CSharpInstanceBridge_DeserializeState;
         public delegate* unmanaged<IntPtr, void> GCHandleBridge_FreeGCHandle;
+        public delegate* unmanaged<IntPtr, godot_bool> GCHandleBridge_GCHandleIsTargetCollectible;
         public delegate* unmanaged<void*, void> DebuggingUtils_GetCurrentStackInfo;
         public delegate* unmanaged<void> DisposablesTracker_OnGodotShuttingDown;
         public delegate* unmanaged<godot_bool, void> GD_OnCoreApiAssemblyLoaded;
@@ -78,6 +79,7 @@ public static ManagedCallbacks Create()
                 CSharpInstanceBridge_SerializeState = &CSharpInstanceBridge.SerializeState,
                 CSharpInstanceBridge_DeserializeState = &CSharpInstanceBridge.DeserializeState,
                 GCHandleBridge_FreeGCHandle = &GCHandleBridge.FreeGCHandle,
+                GCHandleBridge_GCHandleIsTargetCollectible = &GCHandleBridge.GCHandleIsTargetCollectible,
                 DebuggingUtils_GetCurrentStackInfo = &DebuggingUtils.GetCurrentStackInfo,
                 DisposablesTracker_OnGodotShuttingDown = &DisposablesTracker.OnGodotShuttingDown,
                 GD_OnCoreApiAssemblyLoaded = &GD.OnCoreApiAssemblyLoaded,

modules/mono/glue/GodotSharp/GodotSharp/Core/DelegateUtils.cs

@@ -560,6 +560,38 @@ private static bool TryDeserializeMethodInfo(BinaryReader reader,
             return type;
         }
 
+        // Returns true, if unloading the delegate is necessary for assembly unloading to succeed.
+        // This check is not perfect and only intended to prevent things in GodotTools from being reloaded.
+        internal static bool IsDelegateCollectible(Delegate @delegate)
+        {
+            if (@delegate.GetType().IsCollectible)
+                return true;
+
+            if (@delegate is MulticastDelegate multicastDelegate)
+            {
+                Delegate[] invocationList = multicastDelegate.GetInvocationList();
+
+                if (invocationList.Length > 1)
+                {
+                    foreach (Delegate oneDelegate in invocationList)
+                        if (IsDelegateCollectible(oneDelegate))
+                            return true;
+
+                    return false;
+                }
+            }
+
+            if (@delegate.Method.IsCollectible)
+                return true;
+
+            object? target = @delegate.Target;
+
+            if (target is not null && target.GetType().IsCollectible)
+                return true;
+
+            return false;
+        }
+
         internal static class RuntimeTypeConversionHelper
         {
             [SuppressMessage("ReSharper", "RedundantNameQualifier")]

modules/mono/mono_gd/gd_mono.cpp

@@ -488,26 +488,48 @@ bool GDMono::_load_project_assembly() {
 #endif
 
 #ifdef GD_MONO_HOT_RELOAD
+void GDMono::reload_failure() {
+	if (++project_load_failure_count >= (int)GLOBAL_GET("dotnet/project/assembly_reload_attempts")) {
+		// After reloading a project has failed n times in a row, update the path and modification time
+		// to stop any further attempts at loading this assembly, which probably is never going to work anyways.
+		project_load_failure_count = 0;
+
+		ERR_PRINT_ED(".NET: Giving up on assembly reloading. Please restart the editor if unloading was failing.");
+
+		String assembly_name = path::get_csharp_project_name();
+		String assembly_path = GodotSharpDirs::get_res_temp_assemblies_dir().path_join(assembly_name + ".dll");
+		assembly_path = ProjectSettings::get_singleton()->globalize_path(assembly_path);
+		project_assembly_path = assembly_path.simplify_path();
+		project_assembly_modified_time = FileAccess::get_modified_time(assembly_path);
+	}
+}
+
 Error GDMono::reload_project_assemblies() {
 	ERR_FAIL_COND_V(!runtime_initialized, ERR_BUG);
 
 	finalizing_scripts_domain = true;
 
-	CSharpLanguage::get_singleton()->_on_scripts_domain_about_to_unload();
-
 	if (!get_plugin_callbacks().UnloadProjectPluginCallback()) {
-		ERR_FAIL_V_MSG(Error::FAILED, ".NET: Failed to unload assemblies.");
+		ERR_PRINT_ED(".NET: Failed to unload assemblies. Please check https://github.com/godotengine/godot/issues/78513 for more information.");
+		reload_failure();
+		return FAILED;
 	}
 
 	finalizing_scripts_domain = false;
 
 	// Load the project's main assembly. Here, during hot-reloading, we do
 	// consider failing to load the project's main assembly to be an error.
 	if (!_load_project_assembly()) {
-		print_error(".NET: Failed to load project assembly.");
+		ERR_PRINT_ED(".NET: Failed to load project assembly.");
+		reload_failure();
 		return ERR_CANT_OPEN;
 	}
 
+	if (project_load_failure_count > 0) {
+		project_load_failure_count = 0;
+		ERR_PRINT_ED(".NET: Assembly reloading succeeded after failures.");
+	}
+
 	return OK;
 }
 #endif

modules/mono/mono_gd/gd_mono.h

@@ -68,6 +68,7 @@ class GDMono {
 
 	String project_assembly_path;
 	uint64_t project_assembly_modified_time = 0;
+	int project_load_failure_count = 0;
 
 #ifdef TOOLS_ENABLED
 	bool _load_project_assembly();
@@ -144,6 +145,7 @@ class GDMono {
 #endif
 
 #ifdef GD_MONO_HOT_RELOAD
+	void reload_failure();
 	Error reload_project_assemblies();
 #endif
 

modules/mono/mono_gd/gd_mono_cache.cpp

@@ -79,6 +79,7 @@ void update_godot_api_cache(const ManagedCallbacks &p_managed_callbacks) {
 	CHECK_CALLBACK_NOT_NULL(CSharpInstanceBridge, SerializeState);
 	CHECK_CALLBACK_NOT_NULL(CSharpInstanceBridge, DeserializeState);
 	CHECK_CALLBACK_NOT_NULL(GCHandleBridge, FreeGCHandle);
+	CHECK_CALLBACK_NOT_NULL(GCHandleBridge, GCHandleIsTargetCollectible);
 	CHECK_CALLBACK_NOT_NULL(DebuggingUtils, GetCurrentStackInfo);
 	CHECK_CALLBACK_NOT_NULL(DisposablesTracker, OnGodotShuttingDown);
 	CHECK_CALLBACK_NOT_NULL(GD, OnCoreApiAssemblyLoaded);

modules/mono/mono_gd/gd_mono_cache.h

@@ -104,6 +104,7 @@ struct ManagedCallbacks {
 	using FuncCSharpInstanceBridge_SerializeState = void(GD_CLR_STDCALL *)(GCHandleIntPtr, const Dictionary *, const Dictionary *);
 	using FuncCSharpInstanceBridge_DeserializeState = void(GD_CLR_STDCALL *)(GCHandleIntPtr, const Dictionary *, const Dictionary *);
 	using FuncGCHandleBridge_FreeGCHandle = void(GD_CLR_STDCALL *)(GCHandleIntPtr);
+	using FuncGCHandleBridge_GCHandleIsTargetCollectible = bool(GD_CLR_STDCALL *)(GCHandleIntPtr);
 	using FuncDebuggingUtils_GetCurrentStackInfo = void(GD_CLR_STDCALL *)(Vector<ScriptLanguage::StackInfo> *);
 	using FuncDisposablesTracker_OnGodotShuttingDown = void(GD_CLR_STDCALL *)();
 	using FuncGD_OnCoreApiAssemblyLoaded = void(GD_CLR_STDCALL *)(bool);
@@ -138,6 +139,7 @@ struct ManagedCallbacks {
 	FuncCSharpInstanceBridge_SerializeState CSharpInstanceBridge_SerializeState;
 	FuncCSharpInstanceBridge_DeserializeState CSharpInstanceBridge_DeserializeState;
 	FuncGCHandleBridge_FreeGCHandle GCHandleBridge_FreeGCHandle;
+	FuncGCHandleBridge_GCHandleIsTargetCollectible GCHandleBridge_GCHandleIsTargetCollectible;
 	FuncDebuggingUtils_GetCurrentStackInfo DebuggingUtils_GetCurrentStackInfo;
 	FuncDisposablesTracker_OnGodotShuttingDown DisposablesTracker_OnGodotShuttingDown;
 	FuncGD_OnCoreApiAssemblyLoaded GD_OnCoreApiAssemblyLoaded;