ToxicMushroom/Dalamud
1
0
Fork 0
mirror of https://github.com/goatcorp/Dalamud.git synced 2025-12-12 18:27:23 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

fix: prevent some deadlocks in profile management code

Browse source 
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.
This commit is contained in:
goat 2023-06-20 21:55:31 +02:00
parent da8bbf5a28
commit c3fe41640e
No known key found for this signature in database
GPG key ID: 49E2AA8C6A76498B
7 changed files with 107 additions and 88 deletions
Download patch file Download diff file Expand all files Collapse all files

25
Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs
View file

@ -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);

21
Dalamud/Interface/Internal/Windows/PluginInstaller/ProfileManagerWidget.cs
View file

@ -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,8 +413,7 @@ internal class ProfileManagerWidget
if (wantRemovePluginInternalName != null)
{
// TODO: handle error
profile.Remove(wantRemovePluginInternalName, false);
Task.Run(() => profman.ApplyAllWantStates())
Task.Run(() => profile.RemoveAsync(wantRemovePluginInternalName, false))
.ContinueWith(this.installer.DisplayErrorContinuation, Locs.ErrorCouldNotRemove);
}

11
Dalamud/Plugin/Internal/PluginManager.cs
View file

@ -262,6 +262,7 @@ internal partial class PluginManager : IDisposable, IServiceType
/// <summary>
/// Get a disposable that will lock plugin lists while it is not disposed.
/// You must NEVER use this in async code.
/// </summary>
/// <returns>The aforementioned disposable.</returns>
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)
{

38
Dalamud/Plugin/Internal/Profiles/Profile.cs
View file

@ -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
/// <param name="enabled">Whether or not the profile is enabled.</param>
/// <param name="apply">Whether or not the current state should immediately be applied.</param>
/// <exception cref="InvalidOperationException">Thrown when an untoggleable profile is toggled.</exception>
public void SetState(bool enabled, bool apply = true)
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
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<DalamudConfiguration>.Get().QueueSave();
if (apply)
this.manager.ApplyAllWantStates();
await this.manager.ApplyAllWantStatesAsync();
}
/// <summary>
@ -137,11 +139,12 @@ internal class Profile
/// <returns>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.</returns>
public bool? WantsPlugin(string internalName)
{
using var lockScope = this.manager.GetSyncScope();
lock (this)
{
var entry = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName);
return entry?.IsEnabled;
}
}
/// <summary>
/// Add a plugin to this profile with the desired state, or change the state of a plugin in this profile.
@ -150,12 +153,13 @@ internal class Profile
/// <param name="internalName">The internal name of the plugin.</param>
/// <param name="state">Whether or not the plugin should be enabled.</param>
/// <param name="apply">Whether or not the current state should immediately be applied.</param>
public void AddOrUpdate(string internalName, bool state, bool apply = true)
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public async Task AddOrUpdateAsync(string internalName, bool state, bool apply = true)
{
using var lockScope = this.manager.GetSyncScope();
Debug.Assert(!internalName.IsNullOrEmpty(), "!internalName.IsNullOrEmpty()");
lock (this)
{
var existing = this.modelV1.Plugins.FirstOrDefault(x => x.InternalName == internalName);
if (existing != null)
{
@ -169,17 +173,18 @@ internal class Profile
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<DalamudConfiguration>.Get().QueueSave();
if (apply)
this.manager.ApplyAllWantStates();
await this.manager.ApplyAllWantStatesAsync();
}
/// <summary>
@ -188,23 +193,26 @@ internal class Profile
/// </summary>
/// <param name="internalName">The internal name of the plugin.</param>
/// <param name="apply">Whether or not the current state should immediately be applied.</param>
public void Remove(string internalName, bool apply = true)
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
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);
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");
}
// 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<DalamudConfiguration>.Get().QueueSave();
if (apply)
this.manager.ApplyAllWantStates();
await this.manager.ApplyAllWantStatesAsync();
}
}

8
Dalamud/Plugin/Internal/Profiles/ProfileCommandHandler.cs
View file

@ -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)
{

40
Dalamud/Plugin/Internal/Profiles/ProfileManager.cs
View file

@ -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
/// </summary>
public bool IsBusy => this.isBusy;
/// <summary>
/// Get a disposable that will lock the profile list while it is not disposed.
/// </summary>
/// <returns>The aforementioned disposable.</returns>
public ScopedSyncRoot GetSyncScope() => new ScopedSyncRoot(this.profiles);
/// <summary>
/// Check if any enabled profile wants a specific plugin enabled.
/// </summary>
@ -73,13 +66,13 @@ internal class ProfileManager : IServiceType
/// <param name="defaultState">The state the plugin shall be in, if it needs to be added.</param>
/// <param name="addIfNotDeclared">Whether or not the plugin should be added to the default preset, if it's not present in any preset.</param>
/// <returns>Whether or not the plugin shall be enabled.</returns>
public bool GetWantState(string internalName, bool defaultState, bool addIfNotDeclared = true)
{
lock (this.profiles)
public async Task<bool> GetWantStateAsync(string internalName, bool defaultState, bool addIfNotDeclared = true)
{
var want = false;
var wasInAnyProfile = false;
lock (this.profiles)
{
foreach (var profile in this.profiles)
{
var state = profile.WantsPlugin(internalName);
@ -89,17 +82,18 @@ 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);
await this.DefaultProfile.AddOrUpdateAsync(internalName, defaultState, false);
return defaultState;
}
return want;
}
}
/// <summary>
/// Check whether a plugin is declared in any profile.
@ -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.
/// </summary>
public void ApplyAllWantStates()
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
public async Task ApplyAllWantStatesAsync()
{
var pm = Service<PluginManager>.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<string> 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<Task>();
var pm = Service<PluginManager>.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.
/// </remarks>
/// <param name="profile">The profile to delete.</param>
public void DeleteProfile(Profile profile)
/// <returns>A <see cref="Task"/> representing the asynchronous operation.</returns>
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))

2
Dalamud/Plugin/Internal/Types/LocalPlugin.cs
View file

@ -213,7 +213,7 @@ internal class LocalPlugin : IDisposable
/// INCLUDES the default profile.
/// </summary>
public bool IsWantedByAnyProfile =>
Service<ProfileManager>.Get().GetWantState(this.Manifest.InternalName, false, false);
Service<ProfileManager>.Get().GetWantStateAsync(this.Manifest.InternalName, false, false).GetAwaiter().GetResult();
/// <summary>
/// Gets a value indicating whether this plugin's API level is out of date.