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

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;
 
 /// <summary>
@@ -1013,7 +1015,7 @@ internal class DalamudInterface : IInternalDisposableService
 
                     if (ImGui.MenuItem("Scan dev plugins"))
                     {
-                        pluginManager.ScanDevPlugins();
+                        Task.Run(pluginManager.ScanDevPluginsAsync);
                     }
 
                     ImGui.Separator();

Dalamud/Interface/Internal/Windows/PluginInstaller/PluginInstallerWindow.cs

@@ -283,7 +283,7 @@ internal class PluginInstallerWindow : Window, IDisposable
         var pluginManager = Service<PluginManager>.Get();
 
         _ = pluginManager.ReloadPluginMastersAsync();
-        Service<PluginManager>.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();
             }
         }
 

Dalamud/Interface/Internal/Windows/Settings/Widgets/DevPluginsSettingsEntry.cs

@@ -52,7 +52,7 @@ public class DevPluginsSettingsEntry : SettingsEntry
 
         if (this.devPluginLocationsChanged)
         {
-            Service<PluginManager>.Get().ScanDevPlugins();
+            _ = Service<PluginManager>.Get().ScanDevPluginsAsync();
             this.devPluginLocationsChanged = false;
         }
     }

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.
     /// </summary>
-    public void ScanDevPlugins()
+    /// <returns>A <see cref="Task"/> representing the asynchronous operation. This function generally will not block as new plugins aren't loaded.</returns>
+    public async Task ScanDevPluginsAsync()
     {
         // devPlugins are more freeform. Look for any dll and hope to get lucky.
         var devDllFiles = new List<FileInfo>();
@@ -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)
+                await this.LoadPluginAsync(dllFile, manifest, PluginLoadReason.Installer, isDev: true, doNotLoad: true);
-                    .Wait();
                 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
     /// <returns>The loaded plugin.</returns>
     private async Task<LocalPlugin> 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,6 +1572,14 @@ internal class PluginManager : IInternalDisposableService
             throw new Exception("No internal name");
         }
 
+        // 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)
+        {
+            // 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");
+
             if (isDev)
             {
                 Log.Information("Loading dev plugin {Name}", manifest.InternalName);
@@ -1597,6 +1595,11 @@ internal class PluginManager : IInternalDisposableService
                 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}");
+                    Log.Information(ex, "{InternalName}: Dev plugin failed to load", plugin.Manifest.InternalName);
-
-                    // 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
                 }
                 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}");
+                    Log.Information(ex, "{InternalName}: Regular plugin failed to load", plugin.Manifest.InternalName);
-
-                    // 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
                 }
                 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}");
+                    Log.Information(ex, "{InternalName}: Plugin not loaded due to policy", plugin.Manifest.InternalName);
-
-                    // 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
                 }
                 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);

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.
 /// </summary>
-internal class LocalDevPlugin : LocalPlugin
+internal sealed class LocalDevPlugin : LocalPlugin
 {
     private static readonly ModuleLog Log = new("PLUGIN");
 

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
+            Log.Verbose("{Name} ({Guid}): Have type", this.InternalName, this.EffectiveWorkingPluginId);
-            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)));
 
             // 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,40 +615,58 @@ 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.ResetLoader();
-            this.pluginType = null;
-            this.loader.Dispose();
-
             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}");
+            this.ResetLoader();
-            // Something blew up when parsing types, but we still want to look for IDalamudPlugin. Let Load() handle the error.
+            Log.Error(ex, "Could not load one or more types when searching for IDalamudPlugin: {DllFileFullName}", this.DllFile.FullName);
-            this.pluginType = ex.Types.FirstOrDefault(type => type != null && type.IsAssignableTo(typeof(IDalamudPlugin)));
+            throw;
         }
 
-        if (this.pluginType == default)
+        if (this.pluginType == null)
+        {
+            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?.Dispose();
-
+        this.loader = null;
-            Log.Error($"Nothing inherits from IDalamudPlugin: {this.DllFile.FullName}");
-            throw new InvalidPluginException(this.DllFile);
-        }
     }
 
     /// <summary>Clears and disposes all resources associated with the plugin instance.</summary>