From f032adb155a41eaaad2cf9db00ce9a3cd3bb19fb Mon Sep 17 00:00:00 2001 From: goat Date: Wed, 8 Mar 2023 22:37:44 +0100 Subject: [PATCH 1/3] fix: correctly use dependency order to unload, declare all plugin services as deps to PM --- Dalamud/Game/Framework.cs | 5 +- Dalamud/Plugin/Internal/PluginManager.cs | 8 +- Dalamud/ServiceManager.cs | 190 +++++++++++++++++++++-- Dalamud/Service{T}.cs | 45 +++++- 4 files changed, 222 insertions(+), 26 deletions(-) diff --git a/Dalamud/Game/Framework.cs b/Dalamud/Game/Framework.cs index 16bed5696..bc1ec2198 100644 --- a/Dalamud/Game/Framework.cs +++ b/Dalamud/Game/Framework.cs @@ -492,7 +492,10 @@ public sealed class Framework : IDisposable, IServiceType Log.Information("Framework::Destroy!"); Service.Get().Unload(); this.RunPendingTickTasks(); - ServiceManager.UnloadAllServices(); + + // why did we do this here? EntryPoint also does it when the signal is set, what sense does that make + // we should definitely wait for pending tick tasks though + // ServiceManager.UnloadAllServices(); Log.Information("Framework::Destroy OK!"); return this.destroyHook.OriginalDisposeSafe(framework); diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index 68b239c9d..164aa083f 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -32,14 +32,14 @@ namespace Dalamud.Plugin.Internal; /// /// Class responsible for loading and unloading plugins. +/// NOTE: ALL plugin exposed services are marked as dependencies for PluginManager in Service{T}. /// [ServiceManager.EarlyLoadedService] #pragma warning disable SA1015 -// DalamudTextureWrap registers textures to dispose with IM -[InherentDependency] -// DalamudPluginInterface asks to remove chat link handlers -[InherentDependency] +// DalamudTextureWrap registers textures to dispose with IM +[InherentDependency] + #pragma warning restore SA1015 internal partial class PluginManager : IDisposable, IServiceType { diff --git a/Dalamud/ServiceManager.cs b/Dalamud/ServiceManager.cs index d49fe5aa6..92f2563e5 100644 --- a/Dalamud/ServiceManager.cs +++ b/Dalamud/ServiceManager.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Linq; using System.Reflection; @@ -19,6 +20,13 @@ namespace Dalamud; /// internal static class ServiceManager { + /** + * TODO: + * - Unify dependency walking code(load/unload + * - Visualize/output .dot or imgui thing + */ + + /// /// Static log facility for Service{T}, to avoid duplicate instances for different types. /// @@ -27,6 +35,38 @@ internal static class ServiceManager private static readonly TaskCompletionSource BlockingServicesLoadedTaskCompletionSource = new(); private static readonly List LoadedServices = new(); + + /// + /// Kinds of services. + /// + [Flags] + public enum ServiceKind + { + /// + /// Not a service. + /// + None = 0, + + /// + /// Regular service. + /// + ManualService = 1 << 0, + + /// + /// Service that is loaded asynchronously while the game starts. + /// + EarlyLoadedService = 1 << 1, + + /// + /// Service that is loaded before the game starts. + /// + BlockingEarlyLoadedService = 1 << 2, + + /// + /// Service that is loaded automatically when the game starts, synchronously or asynchronously. + /// + AutoLoadService = EarlyLoadedService | BlockingEarlyLoadedService, + } /// /// Gets task that gets completed when all blocking early loading services are done loading. @@ -86,12 +126,34 @@ internal static class ServiceManager var dependencyServicesMap = new Dictionary>(); var getAsyncTaskMap = new Dictionary(); + + // fill getAsyncTaskMap with services that were provided + lock (LoadedServices) + { + foreach (var loadedService in LoadedServices) + { + var getTask = (Task)typeof(Service<>) + .MakeGenericType(loadedService) + .InvokeMember( + "GetAsync", + BindingFlags.InvokeMethod | BindingFlags.Static | BindingFlags.Public, + null, + null, + null); + + Debug.Assert(getTask != null, "Provided service getTask was null"); + + getAsyncTaskMap[typeof(Service<>).MakeGenericType(loadedService)] = getTask; + } + } foreach (var serviceType in Assembly.GetExecutingAssembly().GetTypes()) { - var attr = serviceType.GetCustomAttribute(true)?.GetType(); - if (attr?.IsAssignableTo(typeof(EarlyLoadedService)) != true) + var serviceKind = serviceType.GetServiceKind(); + if (serviceKind == ServiceKind.None) continue; + + Debug.Assert(!serviceKind.HasFlag(ServiceKind.ManualService), "Regular services should never end up here"); var getTask = (Task)typeof(Service<>) .MakeGenericType(serviceType) @@ -102,9 +164,9 @@ internal static class ServiceManager null, null); - if (attr.IsAssignableTo(typeof(BlockingEarlyLoadedService))) + if (serviceKind.HasFlag(ServiceKind.BlockingEarlyLoadedService)) { - getAsyncTaskMap[serviceType] = getTask; + getAsyncTaskMap[typeof(Service<>).MakeGenericType(serviceType)] = getTask; blockingEarlyLoadingServices.Add(serviceType); } else @@ -148,8 +210,29 @@ internal static class ServiceManager { foreach (var serviceType in servicesToLoad) { - if (dependencyServicesMap[serviceType].Any( - x => getAsyncTaskMap.GetValueOrDefault(x)?.IsCompleted == false)) + var hasDeps = true; + foreach (var dependency in dependencyServicesMap[serviceType]) + { + var depServiceKind = dependency.GetServiceKind(); + var depResolveTask = getAsyncTaskMap.GetValueOrDefault(dependency); + + if (depResolveTask == null && (depServiceKind.HasFlag(ServiceKind.EarlyLoadedService) || depServiceKind.HasFlag(ServiceKind.BlockingEarlyLoadedService))) + { + Log.Information("{Type}: {Dependency} has no resolver task, is it early loaded or blocking early loaded?", serviceType.FullName!, dependency.FullName!); + Debug.Assert(false, $"No resolver for dependent service {dependency.FullName}"); + } + else if (depResolveTask is { Status: TaskStatus.Created }) + { + depResolveTask.Start(); + } + else if (depResolveTask is { IsCompleted: false }) + { + Log.Verbose("{Type} waiting for {Dependency}", serviceType.FullName!, dependency.FullName!); + hasDeps = false; + } + } + + if (!hasDeps) continue; tasks.Add((Task)typeof(Service<>) @@ -227,23 +310,104 @@ internal static class ServiceManager return; } - lock (LoadedServices) + var dependencyServicesMap = new Dictionary>(); + var allToUnload = new HashSet(); + var unloadOrder = new List(); + + Log.Information("==== COLLECTING SERVICES TO UNLOAD ===="); + + foreach (var serviceType in Assembly.GetExecutingAssembly().GetTypes()) { - while (LoadedServices.Any()) + if (!serviceType.IsAssignableTo(typeof(IServiceType))) + continue; + + dependencyServicesMap[serviceType] = + ((List)typeof(Service<>) + .MakeGenericType(serviceType) + .InvokeMember( + "GetDependencyServices", + BindingFlags.InvokeMethod | BindingFlags.Static | BindingFlags.Public, + null, + null, + null))! + .Select(x => x.GetGenericArguments()[0]).ToList(); + + /* + Log.Verbose("=> Deps for {Type}", serviceType.FullName!); + foreach (var dependencyService in dependencyServicesMap[serviceType]) { - var serviceType = LoadedServices.Last(); - LoadedServices.RemoveAt(LoadedServices.Count - 1); + Log.Verbose("\t\t=> {Type}", dependencyService.FullName!); + } + */ - typeof(Service<>) - .MakeGenericType(serviceType) + allToUnload.Add(serviceType); + } + + void UnloadService(Type serviceType) + { + if (unloadOrder.Contains(serviceType)) + return; + + var deps = dependencyServicesMap[serviceType]; + foreach (var dep in deps) + { + UnloadService(dep); + } + + unloadOrder.Add(serviceType); + Log.Information("Queue for unload {Type}", serviceType.FullName!); + } + + foreach (var serviceType in allToUnload) + { + UnloadService(serviceType); + } + + Log.Information("==== UNLOADING ALL SERVICES ===="); + + unloadOrder.Reverse(); + foreach (var type in unloadOrder) + { + Log.Verbose("Unload {Type}", type.FullName!); + + typeof(Service<>) + .MakeGenericType(type) .InvokeMember( "Unset", BindingFlags.InvokeMethod | BindingFlags.Static | BindingFlags.NonPublic, null, null, null); - } } + + lock (LoadedServices) + { + LoadedServices.Clear(); + } + } + + /// + /// Get the service type of this type. + /// + /// The type to check. + /// The type of service this type is. + public static ServiceKind GetServiceKind(this Type type) + { + var attr = type.GetCustomAttribute(true)?.GetType(); + if (attr == null) + return ServiceKind.None; + + Debug.Assert( + type.IsAssignableTo(typeof(IServiceType)), + "Service did not inherit from IServiceType"); + + if (attr.IsAssignableTo(typeof(BlockingEarlyLoadedService))) + return ServiceKind.BlockingEarlyLoadedService; + + if (attr.IsAssignableTo(typeof(EarlyLoadedService))) + return ServiceKind.EarlyLoadedService; + + return ServiceKind.ManualService; } /// diff --git a/Dalamud/Service{T}.cs b/Dalamud/Service{T}.cs index 8f805b5af..0e7d75369 100644 --- a/Dalamud/Service{T}.cs +++ b/Dalamud/Service{T}.cs @@ -6,6 +6,7 @@ using System.Threading.Tasks; using Dalamud.IoC; using Dalamud.IoC.Internal; +using Dalamud.Plugin.Internal; using Dalamud.Utility.Timing; using JetBrains.Annotations; @@ -121,18 +122,43 @@ internal static class Service where T : IServiceType public static List GetDependencyServices() { var res = new List(); - res.AddRange(GetServiceConstructor() - .GetParameters() - .Select(x => x.ParameterType)); + + var ctor = GetServiceConstructor(); + if (ctor != null) + { + res.AddRange(ctor + .GetParameters() + .Select(x => x.ParameterType)); + } + res.AddRange(typeof(T) - .GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) - .Select(x => x.FieldType) - .Where(x => x.GetCustomAttribute(true) != null)); + .GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic) + .Select(x => x.FieldType) + .Where(x => x.GetCustomAttribute(true) != null)); + res.AddRange(typeof(T) .GetCustomAttributes() .OfType() .Select(x => x.GetType().GetGenericArguments().First())); + + // HACK: PluginManager needs to depend on ALL plugin exposed services + if (typeof(T) == typeof(PluginManager)) + { + foreach (var serviceType in Assembly.GetExecutingAssembly().GetTypes()) + { + if (!serviceType.IsAssignableTo(typeof(IServiceType))) + continue; + + var attr = serviceType.GetCustomAttribute(true); + if (attr == null) + continue; + + ServiceManager.Log.Verbose("PluginManager MUST depend on {Type}", serviceType.FullName!); + res.Add(serviceType); + } + } + return res .Distinct() .Select(x => typeof(Service<>).MakeGenericType(x)) @@ -228,19 +254,22 @@ internal static class Service where T : IServiceType .GetValue(task); } - private static ConstructorInfo GetServiceConstructor() + private static ConstructorInfo? GetServiceConstructor() { const BindingFlags ctorBindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.CreateInstance | BindingFlags.OptionalParamBinding; return typeof(T) .GetConstructors(ctorBindingFlags) - .Single(x => x.GetCustomAttributes(typeof(ServiceManager.ServiceConstructor), true).Any()); + .SingleOrDefault(x => x.GetCustomAttributes(typeof(ServiceManager.ServiceConstructor), true).Any()); } private static async Task ConstructObject() { var ctor = GetServiceConstructor(); + if (ctor == null) + throw new Exception($"Service \"{typeof(T).FullName}\" had no applicable constructor"); + var args = await Task.WhenAll( ctor.GetParameters().Select(x => ResolveServiceFromTypeAsync(x.ParameterType))); using (Timings.Start($"{typeof(T).Name} Construct")) From 48611dcb74cf8068fa2f8310be9c0ed459610a99 Mon Sep 17 00:00:00 2001 From: goat Date: Wed, 8 Mar 2023 23:13:45 +0100 Subject: [PATCH 2/3] fix: correctly use dependency order to unload, declare all plugin services as deps to PM --- Dalamud/ServiceManager.cs | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/Dalamud/ServiceManager.cs b/Dalamud/ServiceManager.cs index 92f2563e5..c49a78b11 100644 --- a/Dalamud/ServiceManager.cs +++ b/Dalamud/ServiceManager.cs @@ -126,26 +126,6 @@ internal static class ServiceManager var dependencyServicesMap = new Dictionary>(); var getAsyncTaskMap = new Dictionary(); - - // fill getAsyncTaskMap with services that were provided - lock (LoadedServices) - { - foreach (var loadedService in LoadedServices) - { - var getTask = (Task)typeof(Service<>) - .MakeGenericType(loadedService) - .InvokeMember( - "GetAsync", - BindingFlags.InvokeMethod | BindingFlags.Static | BindingFlags.Public, - null, - null, - null); - - Debug.Assert(getTask != null, "Provided service getTask was null"); - - getAsyncTaskMap[typeof(Service<>).MakeGenericType(loadedService)] = getTask; - } - } foreach (var serviceType in Assembly.GetExecutingAssembly().GetTypes()) { @@ -218,16 +198,11 @@ internal static class ServiceManager if (depResolveTask == null && (depServiceKind.HasFlag(ServiceKind.EarlyLoadedService) || depServiceKind.HasFlag(ServiceKind.BlockingEarlyLoadedService))) { - Log.Information("{Type}: {Dependency} has no resolver task, is it early loaded or blocking early loaded?", serviceType.FullName!, dependency.FullName!); + Log.Error("{Type}: {Dependency} has no resolver task, is it early loaded or blocking early loaded?", serviceType.FullName!, dependency.FullName!); Debug.Assert(false, $"No resolver for dependent service {dependency.FullName}"); } - else if (depResolveTask is { Status: TaskStatus.Created }) - { - depResolveTask.Start(); - } else if (depResolveTask is { IsCompleted: false }) { - Log.Verbose("{Type} waiting for {Dependency}", serviceType.FullName!, dependency.FullName!); hasDeps = false; } } @@ -331,14 +306,6 @@ internal static class ServiceManager null, null))! .Select(x => x.GetGenericArguments()[0]).ToList(); - - /* - Log.Verbose("=> Deps for {Type}", serviceType.FullName!); - foreach (var dependencyService in dependencyServicesMap[serviceType]) - { - Log.Verbose("\t\t=> {Type}", dependencyService.FullName!); - } - */ allToUnload.Add(serviceType); } From 01efbf358a0895af1be20016abe9a82be0b10653 Mon Sep 17 00:00:00 2001 From: goat Date: Thu, 9 Mar 2023 11:45:24 +0100 Subject: [PATCH 3/3] fix: wait for services to unload in framework destroy --- Dalamud/Game/Framework.cs | 5 +---- Dalamud/ServiceManager.cs | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Dalamud/Game/Framework.cs b/Dalamud/Game/Framework.cs index bc1ec2198..f2c65723c 100644 --- a/Dalamud/Game/Framework.cs +++ b/Dalamud/Game/Framework.cs @@ -492,10 +492,7 @@ public sealed class Framework : IDisposable, IServiceType Log.Information("Framework::Destroy!"); Service.Get().Unload(); this.RunPendingTickTasks(); - - // why did we do this here? EntryPoint also does it when the signal is set, what sense does that make - // we should definitely wait for pending tick tasks though - // ServiceManager.UnloadAllServices(); + ServiceManager.WaitForServiceUnload(); Log.Information("Framework::Destroy OK!"); return this.destroyHook.OriginalDisposeSafe(framework); diff --git a/Dalamud/ServiceManager.cs b/Dalamud/ServiceManager.cs index c49a78b11..eb4c2eb51 100644 --- a/Dalamud/ServiceManager.cs +++ b/Dalamud/ServiceManager.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.IO; using System.Linq; using System.Reflection; +using System.Threading; using System.Threading.Tasks; using Dalamud.Configuration.Internal; @@ -35,6 +36,8 @@ internal static class ServiceManager private static readonly TaskCompletionSource BlockingServicesLoadedTaskCompletionSource = new(); private static readonly List LoadedServices = new(); + + private static ManualResetEvent unloadResetEvent = new(false); /// /// Kinds of services. @@ -285,6 +288,8 @@ internal static class ServiceManager return; } + unloadResetEvent.Reset(); + var dependencyServicesMap = new Dictionary>(); var allToUnload = new HashSet(); var unloadOrder = new List(); @@ -351,6 +356,16 @@ internal static class ServiceManager { LoadedServices.Clear(); } + + unloadResetEvent.Set(); + } + + /// + /// Wait until all services have been unloaded. + /// + public static void WaitForServiceUnload() + { + unloadResetEvent.WaitOne(); } ///