From 0a8f9b73fb9940d71f2ccb41305c2af04c016a2f Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Sun, 18 Aug 2024 07:58:45 +0900 Subject: [PATCH 1/4] Make ServiceScope IAsyncDisposable ServiceScope.Dispose was not waiting for scoped services to complete disposing. This had an effect of letting a new plugin instance register a DtrBar entry before previous plugin instance's entry got unregistered. This change also cleans up unloading procedure in LocalPlugin. --- Dalamud/Game/Gui/Dtr/DtrBar.cs | 47 +++- Dalamud/IoC/Internal/ServiceScope.cs | 150 ++++++++--- Dalamud/Plugin/Internal/PluginManager.cs | 69 ++--- .../Plugin/Internal/Types/LocalDevPlugin.cs | 7 +- Dalamud/Plugin/Internal/Types/LocalPlugin.cs | 252 +++++++++++------- .../Types/PluginLoaderDisposalMode.cs | 19 ++ Dalamud/Utility/TaskExtensions.cs | 15 ++ 7 files changed, 375 insertions(+), 184 deletions(-) create mode 100644 Dalamud/Plugin/Internal/Types/PluginLoaderDisposalMode.cs diff --git a/Dalamud/Game/Gui/Dtr/DtrBar.cs b/Dalamud/Game/Gui/Dtr/DtrBar.cs index 04e7fea07..4d328d78c 100644 --- a/Dalamud/Game/Gui/Dtr/DtrBar.cs +++ b/Dalamud/Game/Gui/Dtr/DtrBar.cs @@ -114,10 +114,42 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar { if (existingEntry.Title == title) { - existingEntry.ShouldBeRemoved = false; + if (existingEntry.ShouldBeRemoved) + { + if (plugin == existingEntry.OwnerPlugin) + { + Log.Debug( + "Reviving entry: {what}; owner: {plugin}({pluginId})", + title, + plugin?.InternalName, + plugin?.EffectiveWorkingPluginId); + } + else + { + Log.Debug( + "Reviving entry: {what}; old owner: {old}({oldId}); new owner: {new}({newId})", + title, + existingEntry.OwnerPlugin?.InternalName, + existingEntry.OwnerPlugin?.EffectiveWorkingPluginId, + plugin?.InternalName, + plugin?.EffectiveWorkingPluginId); + existingEntry.OwnerPlugin = plugin; + } + + existingEntry.ShouldBeRemoved = false; + } + this.entriesLock.ExitUpgradeableReadLock(); if (plugin == existingEntry.OwnerPlugin) return existingEntry; + + Log.Debug( + "Entry already has a different owner: {what}; owner: {old}({oldId}); requester: {new}({newId})", + title, + existingEntry.OwnerPlugin?.InternalName, + existingEntry.OwnerPlugin?.EffectiveWorkingPluginId, + plugin?.InternalName, + plugin?.EffectiveWorkingPluginId); throw new ArgumentException("An entry with the same title already exists."); } } @@ -125,6 +157,7 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar this.entriesLock.EnterWriteLock(); var entry = new DtrBarEntry(this.configuration, title, null) { Text = text, OwnerPlugin = plugin }; this.entries.Add(entry); + Log.Debug("Adding entry: {what}; owner: {owner}", title, plugin); // Add the entry to the end of the order list, if it's not there already. var dtrOrder = this.configuration.DtrOrder ??= []; @@ -159,14 +192,23 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar { if (!entry.Added) { + Log.Debug("Removing entry immediately because it is not added yet: {what}", entry.Title); this.entriesLock.EnterWriteLock(); this.RemoveEntry(entry); this.entries.Remove(entry); this.entriesReadOnlyCopy = null; this.entriesLock.ExitWriteLock(); } + else if (!entry.ShouldBeRemoved) + { + Log.Debug("Queueing entry for removal: {what}", entry.Title); + entry.Remove(); + } + else + { + Log.Debug("Entry is already marked for removal: {what}", entry.Title); + } - entry.Remove(); break; } } @@ -313,6 +355,7 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar var data = this.entries[i]; if (data.ShouldBeRemoved) { + Log.Debug("Removing entry from Framework.Update: {what}", data.Title); this.entriesLock.EnterWriteLock(); this.entries.RemoveAt(i); this.RemoveEntry(data); diff --git a/Dalamud/IoC/Internal/ServiceScope.cs b/Dalamud/IoC/Internal/ServiceScope.cs index 4fc299f6e..5ce8bc7d0 100644 --- a/Dalamud/IoC/Internal/ServiceScope.cs +++ b/Dalamud/IoC/Internal/ServiceScope.cs @@ -1,16 +1,18 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; -using Serilog; +using Dalamud.Game; +using Dalamud.Utility; namespace Dalamud.IoC.Internal; /// /// Container enabling the creation of scoped services. /// -internal interface IServiceScope : IDisposable +internal interface IServiceScope : IAsyncDisposable { /// /// Register objects that may be injected to scoped services, @@ -47,21 +49,57 @@ internal class ServiceScopeImpl : IServiceScope private readonly List privateScopedObjects = []; private readonly ConcurrentDictionary> scopeCreatedObjects = new(); + private readonly ReaderWriterLockSlim disposeLock = new(LockRecursionPolicy.SupportsRecursion); + private bool disposed; + /// Initializes a new instance of the class. /// The container this scope will use to create services. public ServiceScopeImpl(ServiceContainer container) => this.container = container; /// - public void RegisterPrivateScopes(params object[] scopes) => - this.privateScopedObjects.AddRange(scopes); + public void RegisterPrivateScopes(params object[] scopes) + { + this.disposeLock.EnterReadLock(); + try + { + ObjectDisposedException.ThrowIf(this.disposed, this); + this.privateScopedObjects.AddRange(scopes); + } + finally + { + this.disposeLock.ExitReadLock(); + } + } /// - public Task CreateAsync(Type objectType, params object[] scopedObjects) => - this.container.CreateAsync(objectType, scopedObjects, this); + public Task CreateAsync(Type objectType, params object[] scopedObjects) + { + this.disposeLock.EnterReadLock(); + try + { + ObjectDisposedException.ThrowIf(this.disposed, this); + return this.container.CreateAsync(objectType, scopedObjects, this); + } + finally + { + this.disposeLock.ExitReadLock(); + } + } /// - public Task InjectPropertiesAsync(object instance, params object[] scopedObjects) => - this.container.InjectProperties(instance, scopedObjects, this); + public Task InjectPropertiesAsync(object instance, params object[] scopedObjects) + { + this.disposeLock.EnterReadLock(); + try + { + ObjectDisposedException.ThrowIf(this.disposed, this); + return this.container.InjectProperties(instance, scopedObjects, this); + } + finally + { + this.disposeLock.ExitReadLock(); + } + } /// /// Create a service scoped to this scope, with private scoped objects. @@ -69,39 +107,73 @@ internal class ServiceScopeImpl : IServiceScope /// The type of object to create. /// Additional scoped objects. /// The created object, or null. - public Task CreatePrivateScopedObject(Type objectType, params object[] scopedObjects) => - this.scopeCreatedObjects.GetOrAdd( - objectType, - static (objectType, p) => p.Scope.container.CreateAsync( - objectType, - p.Objects.Concat(p.Scope.privateScopedObjects).ToArray()), - (Scope: this, Objects: scopedObjects)); - - /// - public void Dispose() + public Task CreatePrivateScopedObject(Type objectType, params object[] scopedObjects) { - foreach (var objectTask in this.scopeCreatedObjects) + this.disposeLock.EnterReadLock(); + try { - objectTask.Value.ContinueWith( - static r => - { - if (!r.IsCompletedSuccessfully) - { - if (r.Exception is { } e) - Log.Error(e, "{what}: Failed to load.", nameof(ServiceScopeImpl)); - return; - } - - switch (r.Result) - { - case IInternalDisposableService d: - d.DisposeService(); - break; - case IDisposable d: - d.Dispose(); - break; - } - }); + ObjectDisposedException.ThrowIf(this.disposed, this); + return this.scopeCreatedObjects.GetOrAdd( + objectType, + static (objectType, p) => p.Scope.container.CreateAsync( + objectType, + p.Objects.Concat(p.Scope.privateScopedObjects).ToArray()), + (Scope: this, Objects: scopedObjects)); + } + finally + { + this.disposeLock.ExitReadLock(); } } + + /// + public async ValueTask DisposeAsync() + { + this.disposeLock.EnterWriteLock(); + this.disposed = true; + this.disposeLock.ExitWriteLock(); + + List? exceptions = null; + while (!this.scopeCreatedObjects.IsEmpty) + { + try + { + await Task.WhenAll( + this.scopeCreatedObjects.Keys.Select( + async type => + { + if (!this.scopeCreatedObjects.Remove(type, out var serviceTask)) + return; + + switch (await serviceTask) + { + case IInternalDisposableService d: + d.DisposeService(); + break; + case IAsyncDisposable d: + await d.DisposeAsync(); + break; + case IDisposable d: + d.Dispose(); + break; + } + })); + } + catch (AggregateException ae) + { + exceptions ??= []; + exceptions.AddRange(ae.Flatten().InnerExceptions); + } + } + + // Unless Dalamud is unloading (plugin cannot be reloading at that point), ensure that there are no more + // event callback call in progress when this function returns. Since above service dispose operations should + // have unregistered the event listeners, on next framework tick, none can be running anymore. + // This has an additional effect of ensuring that DtrBar entries are completely removed on return. + // Note that this still does not handle Framework.RunOnTick with specified delays. + await (Service.GetNullable()?.DelayTicks(1) ?? Task.CompletedTask).SuppressException(); + + if (exceptions is not null) + throw new AggregateException(exceptions); + } } diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index 910472f5f..b42b51525 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -375,54 +375,39 @@ internal class PluginManager : IInternalDisposableService /// void IInternalDisposableService.DisposeService() { - var disposablePlugins = - this.installedPluginsList.Where(plugin => plugin.State is PluginState.Loaded or PluginState.LoadError).ToArray(); - if (disposablePlugins.Any()) - { - // Unload them first, just in case some of plugin codes are still running via callbacks initiated externally. - foreach (var plugin in disposablePlugins.Where(plugin => !plugin.Manifest.CanUnloadAsync)) - { - try - { - plugin.UnloadAsync(true, false).Wait(); - } - catch (Exception ex) - { - Log.Error(ex, $"Error unloading {plugin.Name}"); - } - } + DisposeAsync( + this.installedPluginsList + .Where(plugin => plugin.State is PluginState.Loaded or PluginState.LoadError) + .ToArray(), + this.configuration).Wait(); + return; - Task.WaitAll(disposablePlugins - .Where(plugin => plugin.Manifest.CanUnloadAsync) - .Select(plugin => Task.Run(async () => - { - try - { - await plugin.UnloadAsync(true, false); - } - catch (Exception ex) - { - Log.Error(ex, $"Error unloading {plugin.Name}"); - } - })).ToArray()); + static async Task DisposeAsync(LocalPlugin[] disposablePlugins, DalamudConfiguration configuration) + { + if (disposablePlugins.Length == 0) + return; + + // Any unload/dispose operation called from this function log errors on their own. + // Ignore all errors. + + // Unload plugins that requires to be unloaded synchronously, + // just in case some plugin codes are still running via callbacks initiated externally. + foreach (var plugin in disposablePlugins.Where(plugin => !plugin.Manifest.CanUnloadAsync)) + await plugin.UnloadAsync(PluginLoaderDisposalMode.None).SuppressException(); + + // Unload plugins that can be unloaded from any thread. + await Task.WhenAll(disposablePlugins.Select(plugin => plugin.UnloadAsync(PluginLoaderDisposalMode.None))) + .SuppressException(); // Just in case plugins still have tasks running that they didn't cancel when they should have, // give them some time to complete it. - Thread.Sleep(this.configuration.PluginWaitBeforeFree ?? PluginWaitBeforeFreeDefault); + // This helps avoid plugins being reloaded from conflicting with itself of previous instance. + await Task.Delay(configuration.PluginWaitBeforeFree ?? PluginWaitBeforeFreeDefault); // Now that we've waited enough, dispose the whole plugin. - // Since plugins should have been unloaded above, this should be done quickly. - foreach (var plugin in disposablePlugins) - { - try - { - plugin.Dispose(); - } - catch (Exception e) - { - Log.Error(e, $"Error disposing {plugin.Name}"); - } - } + // Since plugins should have been unloaded above, this should complete quickly. + await Task.WhenAll(disposablePlugins.Select(plugin => plugin.DisposeAsync().AsTask())) + .SuppressException(); } // NET8 CHORE diff --git a/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs b/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs index 5a3552199..581bfd724 100644 --- a/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs +++ b/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Threading; using System.Threading.Tasks; @@ -16,7 +15,7 @@ namespace Dalamud.Plugin.Internal.Types; /// This class represents a dev plugin and all facets of its lifecycle. /// The DLL on disk, dependencies, loaded assembly, etc. /// -internal class LocalDevPlugin : LocalPlugin, IDisposable +internal class LocalDevPlugin : LocalPlugin { private static readonly ModuleLog Log = new("PLUGIN"); @@ -101,7 +100,7 @@ internal class LocalDevPlugin : LocalPlugin, IDisposable public List DismissedValidationProblems => this.devSettings.DismissedValidationProblems; /// - public new void Dispose() + public override ValueTask DisposeAsync() { if (this.fileWatcher != null) { @@ -110,7 +109,7 @@ internal class LocalDevPlugin : LocalPlugin, IDisposable this.fileWatcher.Dispose(); } - base.Dispose(); + return base.DisposeAsync(); } /// diff --git a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs index b94c40918..34641fe4b 100644 --- a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs +++ b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs @@ -1,6 +1,8 @@ +using System.Collections.Generic; using System.IO; using System.Linq; using System.Reflection; +using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; @@ -20,7 +22,7 @@ namespace Dalamud.Plugin.Internal.Types; /// This class represents a plugin and all facets of its lifecycle. /// The DLL on disk, dependencies, loaded assembly, etc. /// -internal class LocalPlugin : IDisposable +internal class LocalPlugin : IAsyncDisposable { /// /// The underlying manifest for this plugin. @@ -41,6 +43,8 @@ internal class LocalPlugin : IDisposable private Assembly? pluginAssembly; private Type? pluginType; private IDalamudPlugin? instance; + private IServiceScope? serviceScope; + private DalamudPluginInterface? dalamudInterface; /// /// Initializes a new instance of the class. @@ -107,7 +111,7 @@ internal class LocalPlugin : IDisposable /// /// Gets the associated with this plugin. /// - public DalamudPluginInterface? DalamudInterface { get; private set; } + public DalamudPluginInterface? DalamudInterface => this.dalamudInterface; /// /// Gets the path to the plugin DLL. @@ -220,40 +224,11 @@ internal class LocalPlugin : IDisposable /// /// Gets the service scope for this plugin. /// - public IServiceScope? ServiceScope { get; private set; } + public IServiceScope? ServiceScope => this.serviceScope; /// - public void Dispose() - { - var framework = Service.GetNullable(); - var configuration = Service.Get(); - - var didPluginDispose = false; - if (this.instance != null) - { - didPluginDispose = true; - if (this.manifest.CanUnloadAsync || framework == null) - this.instance.Dispose(); - else - framework.RunOnFrameworkThread(() => this.instance.Dispose()).Wait(); - - this.instance = null; - } - - this.DalamudInterface?.Dispose(); - - this.DalamudInterface = null; - - this.ServiceScope?.Dispose(); - this.ServiceScope = null; - - this.pluginType = null; - this.pluginAssembly = null; - - if (this.loader != null && didPluginDispose) - Thread.Sleep(configuration.PluginWaitBeforeFree ?? PluginManager.PluginWaitBeforeFreeDefault); - this.loader?.Dispose(); - } + public virtual async ValueTask DisposeAsync() => + await this.ClearAndDisposeAllResources(PluginLoaderDisposalMode.ImmediateDispose); /// /// Load this plugin. @@ -263,7 +238,6 @@ internal class LocalPlugin : IDisposable /// A task. public async Task LoadAsync(PluginLoadReason reason, bool reloading = false) { - var framework = await Service.GetAsync(); var ioc = await Service.GetAsync(); var pluginManager = await Service.GetAsync(); var dalamud = await Service.GetAsync(); @@ -300,11 +274,17 @@ internal class LocalPlugin : IDisposable if (!this.IsDev) { throw new InvalidPluginOperationException( - $"Unable to load {this.Name}, unload previously faulted, restart Dalamud"); + $"Unable to load {this.Name}, unload previously faulted, restart Dalamud"); } break; case PluginState.Unloaded: + if (this.instance is not null) + { + throw new InvalidPluginOperationException( + "Plugin should have been unloaded but instance is not cleared"); + } + break; case PluginState.Loading: case PluginState.Unloading: @@ -406,39 +386,30 @@ internal class LocalPlugin : IDisposable // NET8 CHORE // PluginManager.PluginLocations[this.pluginType.Assembly.FullName] = new PluginPatchData(this.DllFile); - this.DalamudInterface = - new DalamudPluginInterface(this, reason); + this.dalamudInterface = new(this, reason); - this.ServiceScope = ioc.GetScope(); - this.ServiceScope.RegisterPrivateScopes(this); // Add this LocalPlugin as a private scope, so services can get it + this.serviceScope = ioc.GetScope(); + this.serviceScope.RegisterPrivateScopes(this); // Add this LocalPlugin as a private scope, so services can get it try { - var forceFrameworkThread = this.manifest.LoadSync && this.manifest.LoadRequiredState is 0 or 1; - var newInstanceTask = forceFrameworkThread ? framework.RunOnFrameworkThread(Create) : Create(); - this.instance = await newInstanceTask.ConfigureAwait(false); - - async Task Create() => - (IDalamudPlugin)await this.ServiceScope!.CreateAsync(this.pluginType!, this.DalamudInterface!); + this.instance = await CreatePluginInstance( + this.manifest, + this.serviceScope, + this.pluginType, + this.dalamudInterface); + this.State = PluginState.Loaded; + Log.Information("Finished loading {PluginName}", this.InternalName); } catch (Exception ex) - { - Log.Error(ex, "Exception during plugin initialization"); - this.instance = null; - } - - if (this.instance == null) { this.State = PluginState.LoadError; - this.UnloadAndDisposeState(); - Log.Error( - "Error while loading {PluginName}, failed to bind and call the plugin constructor", this.InternalName); - return; + ex, + "Error while loading {PluginName}, failed to bind and call the plugin constructor", + this.InternalName); + await this.ClearAndDisposeAllResources(PluginLoaderDisposalMode.ImmediateDispose); } - - this.State = PluginState.Loaded; - Log.Information("Finished loading {PluginName}", this.InternalName); } catch (Exception ex) { @@ -462,14 +433,10 @@ internal class LocalPlugin : IDisposable /// Unload this plugin. This is the same as dispose, but without the "disposed" connotations. This object should stay /// in the plugin list until it has been actually disposed. /// - /// Unload while reloading. - /// Wait before disposing loader. + /// How to dispose loader. /// The task. - public async Task UnloadAsync(bool reloading = false, bool waitBeforeLoaderDispose = true) + public async Task UnloadAsync(PluginLoaderDisposalMode disposalMode = PluginLoaderDisposalMode.WaitBeforeDispose) { - var configuration = Service.Get(); - var framework = Service.GetNullable(); - await this.pluginLoadStateLock.WaitAsync(); try { @@ -498,31 +465,10 @@ internal class LocalPlugin : IDisposable this.State = PluginState.Unloading; Log.Information("Unloading {PluginName}", this.InternalName); - try - { - if (this.manifest.CanUnloadAsync || framework == null) - this.instance?.Dispose(); - else - await framework.RunOnFrameworkThread(() => this.instance?.Dispose()).ConfigureAwait(false); - } - catch (Exception e) + if (await this.ClearAndDisposeAllResources(disposalMode) is { } ex) { this.State = PluginState.UnloadError; - Log.Error(e, "Could not unload {PluginName}, error in plugin dispose", this.InternalName); - return; - } - finally - { - this.instance = null; - this.UnloadAndDisposeState(); - - if (!reloading) - { - if (waitBeforeLoaderDispose && this.loader != null) - await Task.Delay(configuration.PluginWaitBeforeFree ?? PluginManager.PluginWaitBeforeFreeDefault); - this.loader?.Dispose(); - this.loader = null; - } + throw ex; } this.State = PluginState.Unloaded; @@ -549,7 +495,7 @@ internal class LocalPlugin : IDisposable { // Don't unload if we're a dev plugin and have an unload error, this is a bad idea but whatever if (this.IsDev && this.State != PluginState.UnloadError) - await this.UnloadAsync(true); + await this.UnloadAsync(PluginLoaderDisposalMode.None); await this.LoadAsync(PluginLoadReason.Reload, true); } @@ -617,6 +563,26 @@ internal class LocalPlugin : IDisposable { } + /// Creates a new instance of the plugin. + /// Plugin manifest. + /// Service scope. + /// Type of the plugin main class. + /// Instance of . + /// A new instance of the plugin. + private static async Task CreatePluginInstance( + LocalPluginManifest manifest, + IServiceScope scope, + Type type, + DalamudPluginInterface dalamudInterface) + { + var framework = await Service.GetAsync(); + var forceFrameworkThread = manifest.LoadSync && manifest.LoadRequiredState is 0 or 1; + var newInstanceTask = forceFrameworkThread ? framework.RunOnFrameworkThread(Create) : Create(); + return await newInstanceTask.ConfigureAwait(false); + + async Task Create() => (IDalamudPlugin)await scope.CreateAsync(type, dalamudInterface); + } + private static void SetupLoaderConfig(LoaderConfig config) { config.IsUnloadable = true; @@ -688,18 +654,110 @@ internal class LocalPlugin : IDisposable } } - private void UnloadAndDisposeState() + /// Clears and disposes all resources associated with the plugin instance. + /// Whether to clear and dispose . + /// Exceptions, if any occurred. + private async Task ClearAndDisposeAllResources(PluginLoaderDisposalMode disposalMode) { - if (this.instance != null) - throw new InvalidOperationException("Plugin instance should be disposed at this point"); + List? exceptions = null; + Log.Verbose( + "{name}({id}): {fn}(disposalMode={disposalMode})", + this.InternalName, + this.EffectiveWorkingPluginId, + nameof(this.ClearAndDisposeAllResources), + disposalMode); - this.DalamudInterface?.Dispose(); - this.DalamudInterface = null; - - this.ServiceScope?.Dispose(); - this.ServiceScope = null; + // Clear the plugin instance first. + if (!await AttemptCleanup( + nameof(this.instance), + Interlocked.Exchange(ref this.instance, null), + this.manifest, + static async (inst, manifest) => + { + var framework = Service.GetNullable(); + if (manifest.CanUnloadAsync || framework is null) + inst.Dispose(); + else + await framework.RunOnFrameworkThread(inst.Dispose).ConfigureAwait(false); + })) + { + // Plugin was not loaded; loader is not referenced anyway, so no need to wait. + disposalMode = PluginLoaderDisposalMode.ImmediateDispose; + } + // Fields below are expected to be alive until the plugin is (attempted) disposed. + // Clear them after this point. this.pluginType = null; this.pluginAssembly = null; + + await AttemptCleanup( + nameof(this.serviceScope), + Interlocked.Exchange(ref this.serviceScope, null), + 0, + static (x, _) => x.DisposeAsync()); + + await AttemptCleanup( + nameof(this.dalamudInterface), + Interlocked.Exchange(ref this.dalamudInterface, null), + 0, + static (x, _) => + { + x.Dispose(); + return ValueTask.CompletedTask; + }); + + if (disposalMode != PluginLoaderDisposalMode.None) + { + await AttemptCleanup( + nameof(this.loader), + Interlocked.Exchange(ref this.loader, null), + disposalMode == PluginLoaderDisposalMode.WaitBeforeDispose + ? Service.Get().PluginWaitBeforeFree ?? + PluginManager.PluginWaitBeforeFreeDefault + : 0, + static async (ldr, waitBeforeDispose) => + { + // Just in case plugins still have tasks running that they didn't cancel when they should have, + // give them some time to complete it. + // This helps avoid plugins being reloaded from conflicting with itself of previous instance. + await Task.Delay(waitBeforeDispose); + + ldr.Dispose(); + }); + } + + return exceptions is not null + ? (AggregateException)ExceptionDispatchInfo.SetCurrentStackTrace(new AggregateException(exceptions)) + : null; + + async ValueTask AttemptCleanup( + string name, + T? what, + TContext context, + Func cb) + where T : class + { + if (what is null) + return false; + + try + { + await cb.Invoke(what, context); + Log.Verbose("{name}({id}): {what} disposed", this.InternalName, this.EffectiveWorkingPluginId, name); + } + catch (Exception ex) + { + exceptions ??= []; + exceptions.Add(ex); + Log.Error( + ex, + "{name}({id}): Failed to dispose {what}", + this.InternalName, + this.EffectiveWorkingPluginId, + name); + } + + return true; + } } } diff --git a/Dalamud/Plugin/Internal/Types/PluginLoaderDisposalMode.cs b/Dalamud/Plugin/Internal/Types/PluginLoaderDisposalMode.cs new file mode 100644 index 000000000..495205089 --- /dev/null +++ b/Dalamud/Plugin/Internal/Types/PluginLoaderDisposalMode.cs @@ -0,0 +1,19 @@ +using System.Threading.Tasks; + +using Dalamud.Plugin.Internal.Loader; + +namespace Dalamud.Plugin.Internal.Types; + +/// Specify how to dispose . +internal enum PluginLoaderDisposalMode +{ + /// Do not dispose the plugin loader. + None, + + /// Whether to wait a few before disposing the loader, just in case there are s + /// from the plugin that are still running. + WaitBeforeDispose, + + /// Immediately dispose the plugin loader. + ImmediateDispose, +} diff --git a/Dalamud/Utility/TaskExtensions.cs b/Dalamud/Utility/TaskExtensions.cs index a65956325..2855511a1 100644 --- a/Dalamud/Utility/TaskExtensions.cs +++ b/Dalamud/Utility/TaskExtensions.cs @@ -63,6 +63,21 @@ public static class TaskExtensions #pragma warning restore RS0030 } + /// Ignores any exceptions thrown from the task. + /// Task to ignore exceptions. + /// A task that completes when completes in any state. + public static async Task SuppressException(this Task task) + { + try + { + await task; + } + catch + { + // ignore + } + } + private static bool IsWaitingValid(Task task) { // In the case the task has been started with the LongRunning flag, it will not be in the TPL thread pool and we can allow waiting regardless. From 9e95ab8ff72730131485bcc94b749d031be14a86 Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Sun, 18 Aug 2024 08:12:00 +0900 Subject: [PATCH 2/4] fix log --- Dalamud/Game/Gui/Dtr/DtrBar.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Dalamud/Game/Gui/Dtr/DtrBar.cs b/Dalamud/Game/Gui/Dtr/DtrBar.cs index 4d328d78c..f37b3addc 100644 --- a/Dalamud/Game/Gui/Dtr/DtrBar.cs +++ b/Dalamud/Game/Gui/Dtr/DtrBar.cs @@ -157,7 +157,11 @@ internal sealed unsafe class DtrBar : IInternalDisposableService, IDtrBar this.entriesLock.EnterWriteLock(); var entry = new DtrBarEntry(this.configuration, title, null) { Text = text, OwnerPlugin = plugin }; this.entries.Add(entry); - Log.Debug("Adding entry: {what}; owner: {owner}", title, plugin); + Log.Debug( + "Adding entry: {what}; owner: {owner}({id})", + title, + plugin?.InternalName, + plugin?.EffectiveWorkingPluginId); // Add the entry to the end of the order list, if it's not there already. var dtrOrder = this.configuration.DtrOrder ??= []; From 6db7acca201a0447526f39596afa48fc30a444df Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Sun, 18 Aug 2024 08:18:30 +0900 Subject: [PATCH 3/4] Do not throw already unloaded exception on exit --- Dalamud/Plugin/Internal/PluginManager.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index b42b51525..f390664b6 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -396,7 +396,9 @@ internal class PluginManager : IInternalDisposableService await plugin.UnloadAsync(PluginLoaderDisposalMode.None).SuppressException(); // Unload plugins that can be unloaded from any thread. - await Task.WhenAll(disposablePlugins.Select(plugin => plugin.UnloadAsync(PluginLoaderDisposalMode.None))) + await Task.WhenAll( + disposablePlugins.Where(plugin => plugin.Manifest.CanUnloadAsync) + .Select(plugin => plugin.UnloadAsync(PluginLoaderDisposalMode.None))) .SuppressException(); // Just in case plugins still have tasks running that they didn't cancel when they should have, From 27ef05325282d6037300a87900d26bfca9212863 Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Sun, 25 Aug 2024 22:23:58 +0900 Subject: [PATCH 4/4] Update docs --- Dalamud/Utility/TaskExtensions.cs | 42 ++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/Dalamud/Utility/TaskExtensions.cs b/Dalamud/Utility/TaskExtensions.cs index 2855511a1..f03a9e9e9 100644 --- a/Dalamud/Utility/TaskExtensions.cs +++ b/Dalamud/Utility/TaskExtensions.cs @@ -63,20 +63,34 @@ public static class TaskExtensions #pragma warning restore RS0030 } - /// Ignores any exceptions thrown from the task. - /// Task to ignore exceptions. - /// A task that completes when completes in any state. - public static async Task SuppressException(this Task task) - { - try - { - await task; - } - catch - { - // ignore - } - } + /// Creates a new that resolves when completes, ignoring + /// exceptions thrown from the task, if any. + /// Task to await and ignore exceptions on failure. + /// A that completes successfully when completes in any state. + /// + /// Awaiting the returned will always complete without exceptions, but awaiting + /// will throw exceptions if it fails, even after this function is called. + /// + /// + /// Wrong use of this function + /// + /// var task = TaskThrowingException(); + /// task.SuppressException(); + /// await TaskThrowingException(); // This line will throw. + /// + /// + /// + /// Correct use of this function, if waiting for the task + /// await TaskThrowingException().SuppressException(); + /// + /// + /// Fire-and-forget
+ /// If not interested in the execution state of Task (fire-and-forget), simply calling this function will do. + /// This function consumes the task's exception, so that it won't bubble up on later garbage collection. + /// TaskThrowingException().SuppressException(); + /// + /// + public static Task SuppressException(this Task task) => task.ContinueWith(static r => r.Exception); private static bool IsWaitingValid(Task task) {