From e29171cc99e428f550aa6517d975342481f53be8 Mon Sep 17 00:00:00 2001 From: goaaats Date: Thu, 24 Apr 2025 21:58:54 +0200 Subject: [PATCH] Fix race condition in plugin load When opening the installer while boot plugins are still loaded, it may have been possible for plugins to be added to the installed plugins list twice, causing various statekeeping issues --- .../Interface/Internal/DalamudInterface.cs | 4 +- .../PluginInstaller/PluginInstallerWindow.cs | 4 +- .../Widgets/DevPluginsSettingsEntry.cs | 2 +- Dalamud/Plugin/Internal/PluginManager.cs | 84 ++++++++----------- .../Plugin/Internal/Types/LocalDevPlugin.cs | 8 +- Dalamud/Plugin/Internal/Types/LocalPlugin.cs | 63 ++++++++------ 6 files changed, 83 insertions(+), 82 deletions(-) diff --git a/Dalamud/Interface/Internal/DalamudInterface.cs b/Dalamud/Interface/Internal/DalamudInterface.cs index 5d21e954a..42907016f 100644 --- a/Dalamud/Interface/Internal/DalamudInterface.cs +++ b/Dalamud/Interface/Internal/DalamudInterface.cs @@ -46,6 +46,8 @@ using ImPlotNET; using PInvoke; using Serilog.Events; +using Task = System.Threading.Tasks.Task; + namespace Dalamud.Interface.Internal; /// @@ -1013,7 +1015,7 @@ internal class DalamudInterface : IInternalDisposableService if (ImGui.MenuItem("Scan dev plugins")) { - pluginManager.ScanDevPlugins(); + Task.Run(pluginManager.ScanDevPluginsAsync); } ImGui.Separator(); diff --git a/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs b/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs index 317fa86d0..6868827b4 100644 --- a/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs +++ b/Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs @@ -283,7 +283,7 @@ internal class PluginInstallerWindow : Window, IDisposable var pluginManager = Service.Get(); _ = pluginManager.ReloadPluginMastersAsync(); - Service.Get().ScanDevPlugins(); + _ = pluginManager.ScanDevPluginsAsync(); if (!this.isSearchTextPrefilled) { @@ -782,7 +782,7 @@ internal class PluginInstallerWindow : Window, IDisposable ImGui.SameLine(); if (ImGui.Button(Locs.FooterButton_ScanDevPlugins)) { - pluginManager.ScanDevPlugins(); + _ = pluginManager.ScanDevPluginsAsync(); } } diff --git a/Dalamud/Interface/Internal/Windows/Settings/Widgets/DevPluginsSettingsEntry.cs b/Dalamud/Interface/Internal/Windows/Settings/Widgets/DevPluginsSettingsEntry.cs index 4c5dc8b83..d7f8b1ca1 100644 --- a/Dalamud/Interface/Internal/Windows/Settings/Widgets/DevPluginsSettingsEntry.cs +++ b/Dalamud/Interface/Internal/Windows/Settings/Widgets/DevPluginsSettingsEntry.cs @@ -52,7 +52,7 @@ public class DevPluginsSettingsEntry : SettingsEntry if (this.devPluginLocationsChanged) { - Service.Get().ScanDevPlugins(); + _ = Service.Get().ScanDevPluginsAsync(); this.devPluginLocationsChanged = false; } } diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index a20f87241..28fc1fcb1 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -776,7 +776,8 @@ internal class PluginManager : IInternalDisposableService /// only shown as disabled in the installed plugins window. This is a modified version of LoadAllPlugins that works /// a little differently. /// - public void ScanDevPlugins() + /// A representing the asynchronous operation. This function generally will not block as new plugins aren't loaded. + public async Task ScanDevPluginsAsync() { // devPlugins are more freeform. Look for any dll and hope to get lucky. var devDllFiles = new List(); @@ -823,8 +824,7 @@ internal class PluginManager : IInternalDisposableService try { // Add them to the list and let the user decide, nothing is auto-loaded. - this.LoadPluginAsync(dllFile, manifest, PluginLoadReason.Installer, isDev: true, doNotLoad: true) - .Wait(); + await this.LoadPluginAsync(dllFile, manifest, PluginLoadReason.Installer, isDev: true, doNotLoad: true); listChanged = true; } catch (InvalidPluginException) @@ -1188,32 +1188,20 @@ internal class PluginManager : IInternalDisposableService { // Testing exclusive if (manifest.IsTestingExclusive && !this.configuration.DoPluginTest) - { - Log.Verbose($"Testing exclusivity: {manifest.InternalName} - {manifest.AssemblyVersion} - {manifest.TestingAssemblyVersion}"); return false; - } // Applicable version if (manifest.ApplicableVersion < this.dalamud.StartInfo.GameVersion) - { - Log.Verbose($"Game version: {manifest.InternalName} - {manifest.AssemblyVersion} - {manifest.TestingAssemblyVersion}"); return false; - } // API level - we keep the API before this in the installer to show as "outdated" var effectiveApiLevel = this.UseTesting(manifest) && manifest.TestingDalamudApiLevel != null ? manifest.TestingDalamudApiLevel.Value : manifest.DalamudApiLevel; if (effectiveApiLevel < DalamudApiLevel - 1 && !this.LoadAllApiLevels) - { - Log.Verbose($"API Level: {manifest.InternalName} - {manifest.AssemblyVersion} - {manifest.TestingAssemblyVersion}"); return false; - } // Banned if (this.IsManifestBanned(manifest)) - { - Log.Verbose($"Banned: {manifest.InternalName} - {manifest.AssemblyVersion} - {manifest.TestingAssemblyVersion}"); return false; - } return true; } @@ -1572,6 +1560,8 @@ internal class PluginManager : IInternalDisposableService /// The loaded plugin. private async Task LoadPluginAsync(FileInfo dllFile, LocalPluginManifest manifest, PluginLoadReason reason, bool isDev = false, bool isBoot = false, bool doNotLoad = false) { + // TODO: Split this function - it should only take care of adding the plugin to the list, not loading itself, that should be done through the plugin instance + var loadPlugin = !doNotLoad; LocalPlugin? plugin; @@ -1582,21 +1572,34 @@ internal class PluginManager : IInternalDisposableService throw new Exception("No internal name"); } - if (isDev) + // Track the plugin as soon as it is instantiated to prevent it from being loaded twice, + // if the installer or DevPlugin scanner is attempting to add plugins while we are still loading boot plugins + lock (this.pluginListLock) { - Log.Information("Loading dev plugin {Name}", manifest.InternalName); - plugin = new LocalDevPlugin(dllFile, manifest); + // Check if this plugin is already loaded + if (this.installedPluginsList.Any(lp => lp.DllFile.FullName == dllFile.FullName)) + throw new InvalidOperationException("Plugin at the provided path is already loaded"); - // This is a dev plugin - turn ImGui asserts on by default if we haven't chosen yet - // TODO(goat): Re-enable this when we have better tracing for what was rendering when - // this.configuration.ImGuiAssertsEnabledAtStartup ??= true; - } - else - { - Log.Information("Loading plugin {Name}", manifest.InternalName); - plugin = new LocalPlugin(dllFile, manifest); + if (isDev) + { + Log.Information("Loading dev plugin {Name}", manifest.InternalName); + plugin = new LocalDevPlugin(dllFile, manifest); + + // This is a dev plugin - turn ImGui asserts on by default if we haven't chosen yet + // TODO(goat): Re-enable this when we have better tracing for what was rendering when + // this.configuration.ImGuiAssertsEnabledAtStartup ??= true; + } + else + { + Log.Information("Loading plugin {Name}", manifest.InternalName); + plugin = new LocalPlugin(dllFile, manifest); + } + + this.installedPluginsList.Add(plugin); } + Log.Verbose("Starting to load plugin {Name} at {FileLocation}", manifest.InternalName, dllFile.FullName); + // Perform a migration from InternalName to GUIDs. The plugin should definitely have a GUID here. // This will also happen if you are installing a plugin with the installer, and that's intended! // It means that, if you have a profile which has unsatisfied plugins, installing a matching plugin will @@ -1697,43 +1700,34 @@ internal class PluginManager : IInternalDisposableService catch (BannedPluginException) { // Out of date plugins get added so they can be updated. - Log.Information($"Plugin was banned, adding anyways: {dllFile.Name}"); + Log.Information("{InternalName}: Plugin was banned, adding anyways", plugin.Manifest.InternalName); } catch (Exception ex) { if (plugin.IsDev) { // Dev plugins always get added to the list so they can be fiddled with in the UI - Log.Information(ex, $"Dev plugin failed to load, adding anyways: {dllFile.Name}"); - - // NOTE(goat): This can't work - plugins don't "unload" if they fail to load. - // plugin.Disable(); // Disable here, otherwise you can't enable+load later + Log.Information(ex, "{InternalName}: Dev plugin failed to load", plugin.Manifest.InternalName); } else if (plugin.IsOutdated) { // Out of date plugins get added, so they can be updated. - Log.Information(ex, $"Plugin was outdated, adding anyways: {dllFile.Name}"); + Log.Information(ex, "{InternalName}: Plugin was outdated", plugin.Manifest.InternalName); } else if (plugin.IsOrphaned) { // Orphaned plugins get added, so that users aren't confused. - Log.Information(ex, $"Plugin was orphaned, adding anyways: {dllFile.Name}"); + Log.Information(ex, "{InternalName}: Plugin was orphaned", plugin.Manifest.InternalName); } else if (isBoot) { // During boot load, plugins always get added to the list so they can be fiddled with in the UI - Log.Information(ex, $"Regular plugin failed to load, adding anyways: {dllFile.Name}"); - - // NOTE(goat): This can't work - plugins don't "unload" if they fail to load. - // plugin.Disable(); // Disable here, otherwise you can't enable+load later + Log.Information(ex, "{InternalName}: Regular plugin failed to load", plugin.Manifest.InternalName); } else if (!plugin.CheckPolicy()) { // During boot load, plugins always get added to the list so they can be fiddled with in the UI - Log.Information(ex, $"Plugin not loaded due to policy, adding anyways: {dllFile.Name}"); - - // NOTE(goat): This can't work - plugins don't "unload" if they fail to load. - // plugin.Disable(); // Disable here, otherwise you can't enable+load later + Log.Information(ex, "{InternalName}: Plugin not loaded due to policy", plugin.Manifest.InternalName); } else { @@ -1742,14 +1736,6 @@ internal class PluginManager : IInternalDisposableService } } - if (plugin == null) - throw new Exception("Plugin was null when adding to list"); - - lock (this.pluginListLock) - { - this.installedPluginsList.Add(plugin); - } - // Mark as finished loading if (manifest.LoadSync) this.StartupLoadTracking?.Finish(manifest.InternalName); diff --git a/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs b/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs index 581bfd724..b8f2b2708 100644 --- a/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs +++ b/Dalamud/Plugin/Internal/Types/LocalDevPlugin.cs @@ -15,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 +internal sealed class LocalDevPlugin : LocalPlugin { private static readonly ModuleLog Log = new("PLUGIN"); @@ -41,7 +41,7 @@ internal class LocalDevPlugin : LocalPlugin configuration.DevPluginSettings[dllFile.FullName] = this.devSettings = new DevPluginSettings(); configuration.QueueSave(); } - + // Legacy dev plugins might not have this! if (this.devSettings.WorkingPluginId == Guid.Empty) { @@ -85,7 +85,7 @@ internal class LocalDevPlugin : LocalPlugin } } } - + /// /// Gets an ID uniquely identifying this specific instance of a devPlugin. /// @@ -152,7 +152,7 @@ internal class LocalDevPlugin : LocalPlugin if (manifestPath.Exists) this.manifest = LocalPluginManifest.Load(manifestPath) ?? throw new Exception("Could not reload manifest."); } - + /// protected override void OnPreReload() { diff --git a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs index 1b9025538..144e09b47 100644 --- a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs +++ b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs @@ -62,12 +62,13 @@ internal class LocalPlugin : IAsyncDisposable } this.DllFile = dllFile; - this.State = PluginState.Unloaded; // Although it is conditionally used here, we need to set the initial value regardless. this.manifestFile = LocalPluginManifest.GetManifestFile(this.DllFile); this.manifest = manifest; + this.State = PluginState.Unloaded; + var needsSaveDueToLegacyFiles = false; // This converts from the ".disabled" file feature to the manifest instead. @@ -352,19 +353,13 @@ internal class LocalPlugin : IAsyncDisposable } this.loader.Reload(); + this.RefreshAssemblyInformation(); } - // Load the assembly - this.pluginAssembly ??= this.loader.LoadDefaultAssembly(); - - this.AssemblyName = this.pluginAssembly.GetName(); - - // Find the plugin interface implementation. It is guaranteed to exist after checking in the ctor. - this.pluginType ??= this.pluginAssembly.GetTypes() - .First(type => type.IsAssignableTo(typeof(IDalamudPlugin))); + Log.Verbose("{Name} ({Guid}): Have type", this.InternalName, this.EffectiveWorkingPluginId); // Check for any loaded plugins with the same assembly name - var assemblyName = this.pluginAssembly.GetName().Name; + var assemblyName = this.pluginAssembly!.GetName().Name; foreach (var otherPlugin in pluginManager.InstalledPlugins) { // During hot-reloading, this plugin will be in the plugin list, and the instance will have been disposed @@ -376,7 +371,7 @@ internal class LocalPlugin : IAsyncDisposable if (otherPluginAssemblyName == assemblyName && otherPluginAssemblyName != null) { this.State = PluginState.Unloaded; - Log.Debug($"Duplicate assembly: {this.Name}"); + Log.Debug("Duplicate assembly: {Name}", this.InternalName); throw new DuplicatePluginException(assemblyName); } @@ -392,7 +387,7 @@ internal class LocalPlugin : IAsyncDisposable this.instance = await CreatePluginInstance( this.manifest, this.serviceScope, - this.pluginType, + this.pluginType!, this.dalamudInterface); this.State = PluginState.Loaded; Log.Information("Finished loading {PluginName}", this.InternalName); @@ -620,42 +615,60 @@ internal class LocalPlugin : IAsyncDisposable throw; } + this.RefreshAssemblyInformation(); + } + + private void RefreshAssemblyInformation() + { + if (this.loader == null) + throw new InvalidOperationException("No loader available"); + try { this.pluginAssembly = this.loader.LoadDefaultAssembly(); + this.AssemblyName = this.pluginAssembly.GetName(); } catch (Exception ex) { - this.pluginAssembly = null; - this.pluginType = null; - this.loader.Dispose(); - + this.ResetLoader(); Log.Error(ex, $"Not a plugin: {this.DllFile.FullName}"); throw new InvalidPluginException(this.DllFile); } + if (this.pluginAssembly == null) + { + this.ResetLoader(); + Log.Error("Plugin assembly is null: {DllFileFullName}", this.DllFile.FullName); + throw new InvalidPluginException(this.DllFile); + } + try { this.pluginType = this.pluginAssembly.GetTypes().FirstOrDefault(type => type.IsAssignableTo(typeof(IDalamudPlugin))); } catch (ReflectionTypeLoadException ex) { - Log.Error(ex, $"Could not load one or more types when searching for IDalamudPlugin: {this.DllFile.FullName}"); - // Something blew up when parsing types, but we still want to look for IDalamudPlugin. Let Load() handle the error. - this.pluginType = ex.Types.FirstOrDefault(type => type != null && type.IsAssignableTo(typeof(IDalamudPlugin))); + this.ResetLoader(); + Log.Error(ex, "Could not load one or more types when searching for IDalamudPlugin: {DllFileFullName}", this.DllFile.FullName); + throw; } - if (this.pluginType == default) + if (this.pluginType == null) { - this.pluginAssembly = null; - this.pluginType = null; - this.loader.Dispose(); - - Log.Error($"Nothing inherits from IDalamudPlugin: {this.DllFile.FullName}"); + this.ResetLoader(); + Log.Error("Nothing inherits from IDalamudPlugin: {DllFileFullName}", this.DllFile.FullName); throw new InvalidPluginException(this.DllFile); } } + private void ResetLoader() + { + this.pluginAssembly = null; + this.pluginType = null; + this.loader?.Dispose(); + this.loader = null; + } + /// Clears and disposes all resources associated with the plugin instance. /// Whether to clear and dispose . /// Exceptions, if any occurred.