From 28e9d5f156287fda34a919b5ebd59b5ece75f976 Mon Sep 17 00:00:00 2001 From: goat Date: Tue, 20 Jun 2023 19:20:08 +0200 Subject: [PATCH] fix: lock profile manager while changing state --- .../PluginInstaller/PluginInstallerWindow.cs | 2 +- Dalamud/Plugin/Internal/PluginManager.cs | 23 +------ Dalamud/Plugin/Internal/Profiles/Profile.cs | 6 ++ .../Internal/Profiles/ProfileManager.cs | 60 ++++++++++++------- Dalamud/Utility/ScopedSyncRoot.cs | 29 +++++++++ 5 files changed, 77 insertions(+), 43 deletions(-) create mode 100644 Dalamud/Utility/ScopedSyncRoot.cs diff --git a/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs b/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs index af642d9cf..7679212e6 100644 --- a/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs +++ b/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs @@ -2898,7 +2898,7 @@ internal class PluginInstallerWindow : Window, IDisposable private void OnInstalledPluginsChanged() { var pluginManager = Service.Get(); - using var pmLock = pluginManager.LockPluginLists(); + using var pmLock = pluginManager.GetSyncScope(); lock (this.listLock) { diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index 7e16868ad..2f6a0606e 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -264,7 +264,7 @@ internal partial class PluginManager : IDisposable, IServiceType /// Get a disposable that will lock plugin lists while it is not disposed. /// /// The aforementioned disposable. - public IDisposable LockPluginLists() => new PluginListLockScope(this.pluginListLock); + public IDisposable GetSyncScope() => new ScopedSyncRoot(this.pluginListLock); /// /// Print to chat any plugin updates and whether they were successful. @@ -1560,25 +1560,4 @@ internal partial class PluginManager var codebasePatch = typeof(PluginManager).GetMethod(nameof(AssemblyCodeBasePatch), BindingFlags.NonPublic | BindingFlags.Static); this.assemblyCodeBaseMonoHook = new MonoMod.RuntimeDetour.Hook(codebaseTarget, codebasePatch); } - - /// - /// Scope for plugin list locks. - /// - private class PluginListLockScope : IDisposable - { - private readonly object lockObj; - - public PluginListLockScope(object lockObj) - { - this.lockObj = lockObj; - Monitor.Enter(lockObj); - } - - /// - public void Dispose() - { - GC.SuppressFinalize(this); - Monitor.Exit(this.lockObj); - } - } } diff --git a/Dalamud/Plugin/Internal/Profiles/Profile.cs b/Dalamud/Plugin/Internal/Profiles/Profile.cs index bae3e6f95..4921db42b 100644 --- a/Dalamud/Plugin/Internal/Profiles/Profile.cs +++ b/Dalamud/Plugin/Internal/Profiles/Profile.cs @@ -137,6 +137,8 @@ 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; } @@ -150,6 +152,8 @@ internal class Profile /// Whether or not the current state should immediately be applied. public void AddOrUpdate(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); @@ -186,6 +190,8 @@ internal class Profile /// Whether or not the current state should immediately be applied. public void Remove(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}\""); diff --git a/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs b/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs index c996a3f43..6250805b0 100644 --- a/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs +++ b/Dalamud/Plugin/Internal/Profiles/ProfileManager.cs @@ -41,7 +41,14 @@ internal class ProfileManager : IServiceType /// /// Gets the default profile. /// - public Profile DefaultProfile => this.profiles.First(x => x.IsDefaultProfile); + public Profile DefaultProfile + { + get + { + lock (this.profiles) + return this.profiles.First(x => x.IsDefaultProfile); + } + } /// /// Gets all profiles, including the default profile. @@ -53,6 +60,12 @@ 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. /// @@ -62,27 +75,30 @@ internal class ProfileManager : IServiceType /// Whether or not the plugin shall be enabled. public bool GetWantState(string internalName, bool defaultState, bool addIfNotDeclared = true) { - var want = false; - var wasInAnyProfile = false; - - foreach (var profile in this.profiles) + lock (this.profiles) { - var state = profile.WantsPlugin(internalName); - if (state.HasValue) + var want = false; + var wasInAnyProfile = false; + + foreach (var profile in this.profiles) { - want = want || (profile.IsEnabled && state.Value); - wasInAnyProfile = true; + var state = profile.WantsPlugin(internalName); + if (state.HasValue) + { + want = want || (profile.IsEnabled && state.Value); + 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; - } + 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; + return want; + } } /// @@ -91,7 +107,10 @@ internal class ProfileManager : IServiceType /// The internal name of the plugin. /// Whether or not the plugin is in any profile. public bool IsInAnyProfile(string internalName) - => this.profiles.Any(x => x.WantsPlugin(internalName) != null); + { + lock (this.profiles) + return this.profiles.Any(x => x.WantsPlugin(internalName) != null); + } /// /// Check whether a plugin is only in the default profile. @@ -170,8 +189,9 @@ internal class ProfileManager : IServiceType public void ApplyAllWantStates() { var pm = Service.Get(); - using var pmLock = pm.LockPluginLists(); - + using var managerLock = pm.GetSyncScope(); + using var profilesLock = this.GetSyncScope(); + this.isBusy = true; Log.Information("Getting want states..."); diff --git a/Dalamud/Utility/ScopedSyncRoot.cs b/Dalamud/Utility/ScopedSyncRoot.cs new file mode 100644 index 000000000..724581f3f --- /dev/null +++ b/Dalamud/Utility/ScopedSyncRoot.cs @@ -0,0 +1,29 @@ +using System; +using System.Threading; + +namespace Dalamud.Utility; + +/// +/// Scope for plugin list locks. +/// +public class ScopedSyncRoot : IDisposable +{ + private readonly object lockObj; + + /// + /// Initializes a new instance of the class. + /// + /// The object to lock. + public ScopedSyncRoot(object lockObj) + { + this.lockObj = lockObj; + Monitor.Enter(lockObj); + } + + /// + public void Dispose() + { + GC.SuppressFinalize(this); + Monitor.Exit(this.lockObj); + } +}