From c3fe41640e323efa021d9efeeb00b556fe6d19b6 Mon Sep 17 00:00:00 2001 From: goat Date: Tue, 20 Jun 2023 21:55:31 +0200 Subject: [PATCH] fix: prevent some deadlocks in profile management code We can never load/unload plugins synchronously, since they might need to get back on framework thread to unload. Also fixes an issue wherein applying might have gotten stuck if an unload threw an exception. --- .../PluginInstaller/PluginInstallerWindow.cs | 25 ++++--- .../PluginInstaller/ProfileManagerWidget.cs | 23 +++--- Dalamud/Plugin/Internal/PluginManager.cs | 11 +-- Dalamud/Plugin/Internal/Profiles/Profile.cs | 70 +++++++++++-------- .../Profiles/ProfileCommandHandler.cs | 8 +-- .../Internal/Profiles/ProfileManager.cs | 56 +++++++-------- Dalamud/Plugin/Internal/Types/LocalPlugin.cs | 2 +- 7 files changed, 107 insertions(+), 88 deletions(-) diff --git a/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs b/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs index be27768be..7a850f5df 100644 --- a/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs +++ b/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs @@ -2367,12 +2367,12 @@ internal class PluginInstallerWindow : Window, IDisposable { if (inProfile) { - Task.Run(() => profile.AddOrUpdate(plugin.Manifest.InternalName, true)) + Task.Run(() => profile.AddOrUpdateAsync(plugin.Manifest.InternalName, true)) .ContinueWith(this.DisplayErrorContinuation, Locs.Profiles_CouldNotAdd); } else { - Task.Run(() => profile.Remove(plugin.Manifest.InternalName)) + Task.Run(() => profile.RemoveAsync(plugin.Manifest.InternalName)) .ContinueWith(this.DisplayErrorContinuation, Locs.Profiles_CouldNotRemove); } } @@ -2391,14 +2391,17 @@ internal class PluginInstallerWindow : Window, IDisposable if (ImGuiComponents.IconButton(FontAwesomeIcon.Times)) { - profileManager.DefaultProfile.AddOrUpdate(plugin.Manifest.InternalName, plugin.IsLoaded, false); + // TODO: Work this out + Task.Run(() => profileManager.DefaultProfile.AddOrUpdateAsync(plugin.Manifest.InternalName, plugin.IsLoaded, false)) + .GetAwaiter().GetResult(); foreach (var profile in profileManager.Profiles.Where(x => !x.IsDefaultProfile && x.Plugins.Any(y => y.InternalName == plugin.Manifest.InternalName))) { - profile.Remove(plugin.Manifest.InternalName, false); + Task.Run(() => profile.RemoveAsync(plugin.Manifest.InternalName, false)) + .GetAwaiter().GetResult(); } - // TODO error handling - Task.Run(() => profileManager.ApplyAllWantStates()); + Task.Run(profileManager.ApplyAllWantStatesAsync) + .ContinueWith(this.DisplayErrorContinuation, Locs.ErrorModal_ProfileApplyFail); } ImGui.SameLine(); @@ -2448,7 +2451,9 @@ internal class PluginInstallerWindow : Window, IDisposable return; } - profileManager.DefaultProfile.AddOrUpdate(plugin.Manifest.InternalName, false, false); + // TODO: Work this out + Task.Run(() => profileManager.DefaultProfile.AddOrUpdateAsync(plugin.Manifest.InternalName, false, false)) + .GetAwaiter().GetResult(); this.enableDisableStatus = OperationStatus.Complete; notifications.AddNotification(Locs.Notifications_PluginDisabled(plugin.Manifest.Name), Locs.Notifications_PluginDisabledTitle, NotificationType.Success); @@ -2466,7 +2471,9 @@ internal class PluginInstallerWindow : Window, IDisposable plugin.ReloadManifest(); } - profileManager.DefaultProfile.AddOrUpdate(plugin.Manifest.InternalName, true, false); + // TODO: Work this out + Task.Run(() => profileManager.DefaultProfile.AddOrUpdateAsync(plugin.Manifest.InternalName, true, false)) + .GetAwaiter().GetResult(); var loadTask = Task.Run(() => plugin.LoadAsync(PluginLoadReason.Installer)) .ContinueWith( @@ -3282,6 +3289,8 @@ internal class PluginInstallerWindow : Window, IDisposable public static string ErrorModal_UpdaterFatal => Loc.Localize("InstallerUpdaterFatal", "Failed to update plugins.\nPlease restart your game and try again. If this error occurs again, please complain."); + public static string ErrorModal_ProfileApplyFail => Loc.Localize("InstallerProfileApplyFail", "Failed to process collections.\nPlease restart your game and try again. If this error occurs again, please complain."); + public static string ErrorModal_UpdaterFail(int failCount) => Loc.Localize("InstallerUpdaterFail", "Failed to update {0} plugins.\nPlease restart your game and try again. If this error occurs again, please complain.").Format(failCount); public static string ErrorModal_UpdaterFailPartial(int successCount, int failCount) => Loc.Localize("InstallerUpdaterFailPartial", "Updated {0} plugins, failed to update {1}.\nPlease restart your game and try again. If this error occurs again, please complain.").Format(successCount, failCount); diff --git a/Dalamud/Interface/Internal/Windows/PluginInstaller/ProfileManagerWidget.cs b/Dalamud/Interface/Internal/Windows/PluginInstaller/ProfileManagerWidget.cs index 4f92cebb8..df2ec5ce6 100644 --- a/Dalamud/Interface/Internal/Windows/PluginInstaller/ProfileManagerWidget.cs +++ b/Dalamud/Interface/Internal/Windows/PluginInstaller/ProfileManagerWidget.cs @@ -119,7 +119,7 @@ internal class ProfileManagerWidget var isEnabled = profile.IsEnabled; if (ImGuiComponents.ToggleButton($"###toggleButton{profile.Guid}", ref isEnabled)) { - Task.Run(() => profile.SetState(isEnabled)) + Task.Run(() => profile.SetStateAsync(isEnabled)) .ContinueWith(this.installer.DisplayErrorContinuation, Locs.ErrorCouldNotChangeState); } @@ -228,9 +228,7 @@ internal class ProfileManagerWidget if (ImGui.Selectable($"{plugin.Manifest.Name}###selector{plugin.Manifest.InternalName}")) { - // TODO this sucks - profile.AddOrUpdate(plugin.Manifest.InternalName, true, false); - Task.Run(() => profman.ApplyAllWantStates()) + Task.Run(() => profile.AddOrUpdateAsync(plugin.Manifest.InternalName, true, false)) .ContinueWith(this.installer.DisplayErrorContinuation, Locs.ErrorCouldNotChangeState); } } @@ -273,8 +271,12 @@ internal class ProfileManagerWidget this.Reset(); // DeleteProfile() is sync, it doesn't apply and we are modifying the plugins collection. Will throw below when iterating - profman.DeleteProfile(profile); - Task.Run(() => profman.ApplyAllWantStates()) + // TODO: DeleteProfileAsync should probably apply as well + Task.Run(async () => + { + await profman.DeleteProfileAsync(profile); + await profman.ApplyAllWantStatesAsync(); + }) .ContinueWith(t => { this.installer.DisplayErrorContinuation(t, Locs.ErrorCouldNotChangeState); @@ -300,7 +302,7 @@ internal class ProfileManagerWidget var isEnabled = profile.IsEnabled; if (ImGuiComponents.ToggleButton($"###toggleButton{profile.Guid}", ref isEnabled)) { - Task.Run(() => profile.SetState(isEnabled)) + Task.Run(() => profile.SetStateAsync(isEnabled)) .ContinueWith(this.installer.DisplayErrorContinuation, Locs.ErrorCouldNotChangeState); } @@ -391,7 +393,7 @@ internal class ProfileManagerWidget var enabled = plugin.IsEnabled; if (ImGui.Checkbox($"###{this.editingProfileGuid}-{plugin.InternalName}", ref enabled)) { - Task.Run(() => profile.AddOrUpdate(plugin.InternalName, enabled)) + Task.Run(() => profile.AddOrUpdateAsync(plugin.InternalName, enabled)) .ContinueWith(this.installer.DisplayErrorContinuation, Locs.ErrorCouldNotChangeState); } @@ -411,9 +413,8 @@ internal class ProfileManagerWidget if (wantRemovePluginInternalName != null) { // TODO: handle error - profile.Remove(wantRemovePluginInternalName, false); - Task.Run(() => profman.ApplyAllWantStates()) - .ContinueWith(this.installer.DisplayErrorContinuation, Locs.ErrorCouldNotRemove); + Task.Run(() => profile.RemoveAsync(wantRemovePluginInternalName, false)) + .ContinueWith(this.installer.DisplayErrorContinuation, Locs.ErrorCouldNotRemove); } if (!didAny) diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index e3c442473..9b407d724 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -262,6 +262,7 @@ internal partial class PluginManager : IDisposable, IServiceType /// /// Get a disposable that will lock plugin lists while it is not disposed. + /// You must NEVER use this in async code. /// /// The aforementioned disposable. public IDisposable GetSyncScope() => new ScopedSyncRoot(this.pluginListLock); @@ -1290,28 +1291,28 @@ internal partial class PluginManager : IDisposable, IServiceType { // We didn't want this plugin, and StartOnBoot is on. That means we don't want it and it should stay off until manually enabled. Log.Verbose("DevPlugin {Name} disabled and StartOnBoot => disable", probablyInternalNameForThisPurpose); - this.profileManager.DefaultProfile.AddOrUpdate(probablyInternalNameForThisPurpose, false, false); + await this.profileManager.DefaultProfile.AddOrUpdateAsync(probablyInternalNameForThisPurpose, false, false); loadPlugin = false; } else if (wantsInDefaultProfile == true && devPlugin.StartOnBoot) { // We wanted this plugin, and StartOnBoot is on. That means we actually do want it. Log.Verbose("DevPlugin {Name} enabled and StartOnBoot => enable", probablyInternalNameForThisPurpose); - this.profileManager.DefaultProfile.AddOrUpdate(probablyInternalNameForThisPurpose, true, false); + await this.profileManager.DefaultProfile.AddOrUpdateAsync(probablyInternalNameForThisPurpose, true, false); loadPlugin = !doNotLoad; } else if (wantsInDefaultProfile == true && !devPlugin.StartOnBoot) { // We wanted this plugin, but StartOnBoot is off. This means we don't want it anymore. Log.Verbose("DevPlugin {Name} enabled and !StartOnBoot => disable", probablyInternalNameForThisPurpose); - this.profileManager.DefaultProfile.AddOrUpdate(probablyInternalNameForThisPurpose, false, false); + await this.profileManager.DefaultProfile.AddOrUpdateAsync(probablyInternalNameForThisPurpose, false, false); loadPlugin = false; } else if (wantsInDefaultProfile == false && !devPlugin.StartOnBoot) { // We didn't want this plugin, and StartOnBoot is off. We don't want it. Log.Verbose("DevPlugin {Name} disabled and !StartOnBoot => disable", probablyInternalNameForThisPurpose); - this.profileManager.DefaultProfile.AddOrUpdate(probablyInternalNameForThisPurpose, false, false); + await this.profileManager.DefaultProfile.AddOrUpdateAsync(probablyInternalNameForThisPurpose, false, false); loadPlugin = false; } @@ -1328,7 +1329,7 @@ internal partial class PluginManager : IDisposable, IServiceType #pragma warning restore CS0618 // Need to do this here, so plugins that don't load are still added to the default profile - var wantToLoad = this.profileManager.GetWantState(plugin.Manifest.InternalName, defaultState); + var wantToLoad = await this.profileManager.GetWantStateAsync(plugin.Manifest.InternalName, defaultState); if (loadPlugin) { diff --git a/Dalamud/Plugin/Internal/Profiles/Profile.cs b/Dalamud/Plugin/Internal/Profiles/Profile.cs index 4921db42b..29ab64fa7 100644 --- a/Dalamud/Plugin/Internal/Profiles/Profile.cs +++ b/Dalamud/Plugin/Internal/Profiles/Profile.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Threading.Tasks; using Dalamud.Configuration.Internal; using Dalamud.Logging.Internal; @@ -115,7 +116,8 @@ internal class Profile /// Whether or not the profile is enabled. /// Whether or not the current state should immediately be applied. /// Thrown when an untoggleable profile is toggled. - public void SetState(bool enabled, bool apply = true) + /// A representing the asynchronous operation. + public async Task SetStateAsync(bool enabled, bool apply = true) { if (this.IsDefaultProfile) throw new InvalidOperationException("Cannot set state of default profile"); @@ -127,7 +129,7 @@ internal class Profile Service.Get().QueueSave(); if (apply) - this.manager.ApplyAllWantStates(); + await this.manager.ApplyAllWantStatesAsync(); } /// @@ -137,10 +139,11 @@ internal class Profile /// Null if this profile does not declare the plugin, true if the profile declares the plugin and wants it enabled, false if the profile declares the plugin and does not want it enabled. public bool? WantsPlugin(string internalName) { - using var lockScope = this.manager.GetSyncScope(); - - var entry = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName); - return entry?.IsEnabled; + lock (this) + { + var entry = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName); + return entry?.IsEnabled; + } } /// @@ -150,36 +153,38 @@ internal class Profile /// The internal name of the plugin. /// Whether or not the plugin should be enabled. /// Whether or not the current state should immediately be applied. - public void AddOrUpdate(string internalName, bool state, bool apply = true) + /// A representing the asynchronous operation. + public async Task AddOrUpdateAsync(string internalName, bool state, bool apply = true) { - using var lockScope = this.manager.GetSyncScope(); - Debug.Assert(!internalName.IsNullOrEmpty(), "!internalName.IsNullOrEmpty()"); - var existing = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName); - if (existing != null) + lock (this) { - existing.IsEnabled = state; - } - else - { - this.modelV1.Plugins.Add(new ProfileModelV1.ProfileModelV1Plugin + var existing = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName); + if (existing != null) { - InternalName = internalName, - IsEnabled = state, - }); + existing.IsEnabled = state; + } + else + { + this.modelV1.Plugins.Add(new ProfileModelV1.ProfileModelV1Plugin + { + InternalName = internalName, + IsEnabled = state, + }); + } } // We need to remove this plugin from the default profile, if it declares it. if (!this.IsDefaultProfile && this.manager.DefaultProfile.WantsPlugin(internalName) != null) { - this.manager.DefaultProfile.Remove(internalName, false); + await this.manager.DefaultProfile.RemoveAsync(internalName, false); } Service.Get().QueueSave(); if (apply) - this.manager.ApplyAllWantStates(); + await this.manager.ApplyAllWantStatesAsync(); } /// @@ -188,23 +193,26 @@ internal class Profile /// /// The internal name of the plugin. /// Whether or not the current state should immediately be applied. - public void Remove(string internalName, bool apply = true) + /// A representing the asynchronous operation. + public async Task RemoveAsync(string internalName, bool apply = true) { - using var lockScope = this.manager.GetSyncScope(); - - var entry = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName); - if (entry == null) - throw new ArgumentException($"No plugin \"{internalName}\" in profile \"{this.Guid}\""); + ProfileModelV1.ProfileModelV1Plugin entry; + lock (this) + { + entry = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName); + if (entry == null) + throw new ArgumentException($"No plugin \"{internalName}\" in profile \"{this.Guid}\""); - if (!this.modelV1.Plugins.Remove(entry)) - throw new Exception("Couldn't remove plugin from model collection"); + if (!this.modelV1.Plugins.Remove(entry)) + throw new Exception("Couldn't remove plugin from model collection"); + } // We need to add this plugin back to the default profile, if we were the last profile to have it. if (!this.manager.IsInAnyProfile(internalName)) { if (!this.IsDefaultProfile) { - this.manager.DefaultProfile.AddOrUpdate(internalName, entry.IsEnabled, false); + await this.manager.DefaultProfile.AddOrUpdateAsync(internalName, entry.IsEnabled, false); } else { @@ -215,6 +223,6 @@ internal class Profile Service.Get().QueueSave(); if (apply) - this.manager.ApplyAllWantStates(); + await this.manager.ApplyAllWantStatesAsync(); } } diff --git a/Dalamud/Plugin/Internal/Profiles/ProfileCommandHandler.cs b/Dalamud/Plugin/Internal/Profiles/ProfileCommandHandler.cs index 81e63e0bc..8ea55856c 100644 --- a/Dalamud/Plugin/Internal/Profiles/ProfileCommandHandler.cs +++ b/Dalamud/Plugin/Internal/Profiles/ProfileCommandHandler.cs @@ -96,14 +96,14 @@ internal class ProfileCommandHandler : IServiceType, IDisposable { case ProfileOp.Enable: if (!profile.IsEnabled) - profile.SetState(true, false); + Task.Run(() => profile.SetStateAsync(true, false)).GetAwaiter().GetResult(); break; case ProfileOp.Disable: if (profile.IsEnabled) - profile.SetState(false, false); + Task.Run(() => profile.SetStateAsync(false, false)).GetAwaiter().GetResult(); break; case ProfileOp.Toggle: - profile.SetState(!profile.IsEnabled, false); + Task.Run(() => profile.SetStateAsync(!profile.IsEnabled, false)).GetAwaiter().GetResult(); break; default: throw new ArgumentOutOfRangeException(); @@ -118,7 +118,7 @@ internal class ProfileCommandHandler : IServiceType, IDisposable this.chat.Print(Loc.Localize("ProfileCommandsDisabling", "Disabling collection \"{0}\"...").Format(profile.Name)); } - Task.Run(() => this.profileManager.ApplyAllWantStates()).ContinueWith(t => + Task.Run(this.profileManager.ApplyAllWantStatesAsync).ContinueWith(t => { if (!t.IsCompletedSuccessfully && t.Exception != null) { diff --git a/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs b/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs index 6250805b0..a3601c773 100644 --- a/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs +++ b/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.Globalization; using System.Linq; using System.Text.RegularExpressions; @@ -60,12 +59,6 @@ internal class ProfileManager : IServiceType /// public bool IsBusy => this.isBusy; - /// - /// Get a disposable that will lock the profile list while it is not disposed. - /// - /// The aforementioned disposable. - public ScopedSyncRoot GetSyncScope() => new ScopedSyncRoot(this.profiles); - /// /// Check if any enabled profile wants a specific plugin enabled. /// @@ -73,13 +66,13 @@ internal class ProfileManager : IServiceType /// The state the plugin shall be in, if it needs to be added. /// Whether or not the plugin should be added to the default preset, if it's not present in any preset. /// Whether or not the plugin shall be enabled. - public bool GetWantState(string internalName, bool defaultState, bool addIfNotDeclared = true) + public async Task GetWantStateAsync(string internalName, bool defaultState, bool addIfNotDeclared = true) { + var want = false; + var wasInAnyProfile = false; + lock (this.profiles) { - var want = false; - var wasInAnyProfile = false; - foreach (var profile in this.profiles) { var state = profile.WantsPlugin(internalName); @@ -89,16 +82,17 @@ internal class ProfileManager : IServiceType wasInAnyProfile = true; } } - - if (!wasInAnyProfile && addIfNotDeclared) - { - Log.Warning("{Name} was not in any profile, adding to default with {Default}", internalName, defaultState); - this.DefaultProfile.AddOrUpdate(internalName, defaultState, false); - return defaultState; - } - - return want; } + + if (!wasInAnyProfile && addIfNotDeclared) + { + Log.Warning("{Name} was not in any profile, adding to default with {Default}", internalName, defaultState); + await this.DefaultProfile.AddOrUpdateAsync(internalName, defaultState, false); + + return defaultState; + } + + return want; } /// @@ -186,20 +180,24 @@ internal class ProfileManager : IServiceType /// Go through all profiles and plugins, and enable/disable plugins they want active. /// This will block until all plugins have been loaded/reloaded. /// - public void ApplyAllWantStates() + /// A representing the asynchronous operation. + public async Task ApplyAllWantStatesAsync() { - var pm = Service.Get(); - using var managerLock = pm.GetSyncScope(); - using var profilesLock = this.GetSyncScope(); + if (this.isBusy) + throw new Exception("Already busy, this must not run in parallel. Check before starting another apply!"); this.isBusy = true; Log.Information("Getting want states..."); - var wantActive = this.profiles + List wantActive; + lock (this.profiles) + { + wantActive = this.profiles .Where(x => x.IsEnabled) .SelectMany(profile => profile.Plugins.Where(plugin => plugin.IsEnabled) .Select(plugin => plugin.InternalName)) .Distinct().ToList(); + } foreach (var internalName in wantActive) { @@ -210,6 +208,7 @@ internal class ProfileManager : IServiceType var tasks = new List(); + var pm = Service.Get(); foreach (var installedPlugin in pm.InstalledPlugins) { var wantThis = wantActive.Contains(installedPlugin.Manifest.InternalName); @@ -237,7 +236,7 @@ internal class ProfileManager : IServiceType // This is probably not ideal... Might need to rethink the error handling strategy for this. try { - Task.WaitAll(tasks.ToArray()); + await Task.WhenAll(tasks.ToArray()); } catch (Exception e) { @@ -255,12 +254,13 @@ internal class ProfileManager : IServiceType /// You should definitely apply states after this. It doesn't do it for you. /// /// The profile to delete. - public void DeleteProfile(Profile profile) + /// A representing the asynchronous operation. + public async Task DeleteProfileAsync(Profile profile) { // We need to remove all plugins from the profile first, so that they are re-added to the default profile if needed foreach (var plugin in profile.Plugins.ToArray()) { - profile.Remove(plugin.InternalName, false); + await profile.RemoveAsync(plugin.InternalName, false); } if (!this.config.SavedProfiles!.Remove(profile.Model)) diff --git a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs index 1af2165da..14ae4a0c0 100644 --- a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs +++ b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs @@ -213,7 +213,7 @@ internal class LocalPlugin : IDisposable /// INCLUDES the default profile. /// public bool IsWantedByAnyProfile => - Service.Get().GetWantState(this.Manifest.InternalName, false, false); + Service.Get().GetWantStateAsync(this.Manifest.InternalName, false, false).GetAwaiter().GetResult(); /// /// Gets a value indicating whether this plugin's API level is out of date.