From e0f644a48da49843ad63a06982af85f13783899c Mon Sep 17 00:00:00 2001 From: RedworkDE <10944644+RedworkDE@users.noreply.github.com> Date: Fri, 31 Mar 2023 11:53:16 +0200 Subject: [PATCH] C#: Fix editor integration breaking and causing error spam when reloading assemblies fails - Do not reload scripts from non-collectible assemblies - Do not load GodotTools as collectible - Do not attempt to reload the same project assembly forever --- doc/classes/ProjectSettings.xml | 3 ++ main/main.cpp | 1 + modules/mono/csharp_script.cpp | 46 +++++++++++-------- modules/mono/csharp_script.h | 1 - .../mono/glue/GodotSharp/GodotPlugins/Main.cs | 44 ++++++++++-------- .../GodotSharp/Core/Bridge/GCHandleBridge.cs | 21 +++++++++ .../Core/Bridge/ManagedCallbacks.cs | 2 + .../GodotSharp/Core/DelegateUtils.cs | 32 +++++++++++++ modules/mono/mono_gd/gd_mono.cpp | 30 ++++++++++-- modules/mono/mono_gd/gd_mono.h | 2 + modules/mono/mono_gd/gd_mono_cache.cpp | 1 + modules/mono/mono_gd/gd_mono_cache.h | 2 + 12 files changed, 142 insertions(+), 43 deletions(-) diff --git a/doc/classes/ProjectSettings.xml b/doc/classes/ProjectSettings.xml index ce36a0336d2..21d5c30ae99 100644 --- a/doc/classes/ProjectSettings.xml +++ b/doc/classes/ProjectSettings.xml @@ -818,6 +818,9 @@ 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. + + 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. + 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. diff --git a/main/main.cpp b/main/main.cpp index 0fb6813c6b6..f6c4df583ba 100644 --- a/main/main.cpp +++ b/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; diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp index bc26352e9c2..d9c372e930b 100644 --- a/modules/mono/csharp_script.cpp +++ b/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 *elem = script_list.first(); elem; elem = elem->next()) { - // Cast to CSharpScript to avoid being erased by accident - scripts.push_back(Ref(elem->self())); + bool is_reloadable = false; + for (Object *obj : elem->self()->instances) { + ERR_CONTINUE(!obj->get_script_instance()); + CSharpInstance *csi = static_cast(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(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 &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 *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 diff --git a/modules/mono/csharp_script.h b/modules/mono/csharp_script.h index cfdf8ae6f8b..9802067b463 100644 --- a/modules/mono/csharp_script.h +++ b/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; diff --git a/modules/mono/glue/GodotSharp/GodotPlugins/Main.cs b/modules/mono/glue/GodotSharp/GodotPlugins/Main.cs index 2a72b7c53ed..6117ae17eaa 100644 --- a/modules/mono/glue/GodotSharp/GodotPlugins/Main.cs +++ b/modules/mono/glue/GodotSharp/GodotPlugins/Main.cs @@ -21,6 +21,13 @@ namespace GodotPlugins 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 @@ namespace GodotPlugins 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 @@ namespace GodotPlugins 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 @@ namespace GodotPlugins 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 @@ namespace GodotPlugins 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 @@ namespace GodotPlugins Console.WriteLine("Assembly load context unloaded successfully."); + pluginLoadContext = null; return true; } catch (Exception e) diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/GCHandleBridge.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/GCHandleBridge.cs index 456a118b90a..8217572648b 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/GCHandleBridge.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/GCHandleBridge.cs @@ -18,5 +18,26 @@ namespace Godot.Bridge 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; + } + } } } diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ManagedCallbacks.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ManagedCallbacks.cs index 0571515e612..109643c2d40 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ManagedCallbacks.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ManagedCallbacks.cs @@ -38,6 +38,7 @@ namespace Godot.Bridge public delegate* unmanaged CSharpInstanceBridge_SerializeState; public delegate* unmanaged CSharpInstanceBridge_DeserializeState; public delegate* unmanaged GCHandleBridge_FreeGCHandle; + public delegate* unmanaged GCHandleBridge_GCHandleIsTargetCollectible; public delegate* unmanaged DebuggingUtils_GetCurrentStackInfo; public delegate* unmanaged DisposablesTracker_OnGodotShuttingDown; public delegate* unmanaged GD_OnCoreApiAssemblyLoaded; @@ -78,6 +79,7 @@ namespace Godot.Bridge 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, diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/DelegateUtils.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/DelegateUtils.cs index 279dadf425f..5d03379430e 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/DelegateUtils.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/DelegateUtils.cs @@ -560,6 +560,38 @@ namespace Godot 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")] diff --git a/modules/mono/mono_gd/gd_mono.cpp b/modules/mono/mono_gd/gd_mono.cpp index 92fa30e5e8b..4337478370a 100644 --- a/modules/mono/mono_gd/gd_mono.cpp +++ b/modules/mono/mono_gd/gd_mono.cpp @@ -488,15 +488,31 @@ 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; @@ -504,10 +520,16 @@ Error GDMono::reload_project_assemblies() { // 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 diff --git a/modules/mono/mono_gd/gd_mono.h b/modules/mono/mono_gd/gd_mono.h index 398f94d9246..c629ab2eff6 100644 --- a/modules/mono/mono_gd/gd_mono.h +++ b/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 @@ public: #endif #ifdef GD_MONO_HOT_RELOAD + void reload_failure(); Error reload_project_assemblies(); #endif diff --git a/modules/mono/mono_gd/gd_mono_cache.cpp b/modules/mono/mono_gd/gd_mono_cache.cpp index e254484df9d..8fdf163b26d 100644 --- a/modules/mono/mono_gd/gd_mono_cache.cpp +++ b/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); diff --git a/modules/mono/mono_gd/gd_mono_cache.h b/modules/mono/mono_gd/gd_mono_cache.h index 9201da7cae4..f604e4d681a 100644 --- a/modules/mono/mono_gd/gd_mono_cache.h +++ b/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 *); 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;