Plugins - Reworked most of the activation logic

src/Artemis.Core/Plugins/Plugin.cs

@@ -1,18 +1,131 @@
-namespace Artemis.Core
+using System;
+using System.Collections.Generic;
+using System.Collections.ObjectModel;
+using System.IO;
+using System.Linq;
+using System.Reflection;
+using Artemis.Storage.Entities.Plugins;
+using McMaster.NETCore.Plugins;
+using Ninject;
+using Stylet;
+
+namespace Artemis.Core
 {
     /// <summary>
     ///     Represents a plugin
     /// </summary>
-    public abstract class Plugin
+    public class Plugin : PropertyChangedBase, IDisposable
     {
+        private readonly List<PluginImplementation> _implementations;
+
+        private bool _isEnabled;
+
+        internal Plugin(PluginInfo info, DirectoryInfo directory)
+        {
+            Info = info;
+            Directory = directory;
+
+            _implementations = new List<PluginImplementation>();
+        }
+
+        /// <summary>
+        ///     Gets the plugin GUID
+        /// </summary>
+        public Guid Guid => Info.Guid;
+
         /// <summary>
         ///     Gets the plugin info related to this plugin
         /// </summary>
-        public PluginInfo Info { get; internal set; }
+        public PluginInfo Info { get; }
+
+        /// <summary>
+        ///     The plugins root directory
+        /// </summary>
+        public DirectoryInfo Directory { get; }
 
         /// <summary>
         ///     Gets or sets a configuration dialog for this plugin that is accessible in the UI under Settings > Plugins
         /// </summary>
-        public PluginConfigurationDialog ConfigurationDialog { get; protected set; }
+        public PluginConfigurationDialog? ConfigurationDialog { get; protected set; }
+        
+        /// <summary>
+        ///     Indicates whether the user enabled the plugin or not
+        /// </summary>
+        public bool IsEnabled
+        {
+            get => _isEnabled;
+            private set => SetAndNotify(ref _isEnabled, value);
+        }
+
+        /// <summary>
+        ///     Gets a read-only collection of all implementations this plugin provides
+        /// </summary>
+        public ReadOnlyCollection<PluginImplementation> Implementations => _implementations.AsReadOnly();
+
+        /// <summary>
+        ///     The assembly the plugin code lives in
+        /// </summary>
+        internal Assembly? Assembly { get; set; }
+
+        /// <summary>
+        ///     The Ninject kernel of the plugin
+        /// </summary>
+        internal IKernel? Kernel { get; set; }
+
+        /// <summary>
+        ///     The PluginLoader backing this plugin
+        /// </summary>
+        internal PluginLoader? PluginLoader { get; set; }
+
+        /// <summary>
+        ///     The entity representing the plugin
+        /// </summary>
+        internal PluginEntity? Entity { get; set; }
+
+        /// <summary>
+        ///     Resolves the relative path provided in the <paramref name="path" /> parameter to an absolute path
+        /// </summary>
+        /// <param name="path">The path to resolve</param>
+        /// <returns>An absolute path pointing to the provided relative path</returns>
+        public string? ResolveRelativePath(string path)
+        {
+            return path == null ? null : Path.Combine(Directory.FullName, path);
+        }
+
+        internal void ApplyToEntity()
+        {
+            Entity.Id = Guid;
+            Entity.IsEnabled = IsEnabled;
+        }
+
+        internal void AddImplementation(PluginImplementation implementation)
+        {
+            implementation.Plugin = this;
+            _implementations.Add(implementation);
+        }
+
+        public void SetEnabled(bool enable)
+        {
+            if (IsEnabled == enable)
+                return;
+
+            if (!enable && Implementations.Any(e => e.IsEnabled))
+                throw new ArtemisCoreException("Cannot disable this plugin because it still has enabled implementations");
+        }
+
+        /// <inheritdoc />
+        public override string ToString()
+        {
+            return Info.ToString();
+        }
+
+        public void Dispose()
+        {
+            foreach (PluginImplementation pluginImplementation in Implementations) 
+                pluginImplementation.Dispose();
+
+            Kernel?.Dispose();
+            PluginLoader?.Dispose();
+        }
     }
 }

src/Artemis.Core/Plugins/PluginImplementation.cs

@@ -1,27 +1,44 @@
 using System;
+using System.IO;
+using System.Threading;
 using System.Threading.Tasks;
+using Artemis.Storage.Entities.Plugins;
+using Stylet;
 
 namespace Artemis.Core
 {
     /// <summary>
     ///     Represents an implementation of a certain type provided by a plugin
     /// </summary>
-    public abstract class PluginImplementation : IDisposable
+    public abstract class PluginImplementation : PropertyChangedBase, IDisposable
     {
+        private Exception? _loadException;
+        private bool _isEnabled;
+
         /// <summary>
         ///     Gets the plugin that provides this implementation
         /// </summary>
-        public Plugin Plugin { get; internal set; }
-
-        /// <summary>
-        ///     Gets the plugin info related to this plugin
-        /// </summary>
-        public PluginInfo PluginInfo { get; internal set; }
+        public Plugin? Plugin { get; internal set; }
 
         /// <summary>
         ///     Gets whether the plugin is enabled
         /// </summary>
-        public bool IsEnabled { get; internal set; }
+        public bool IsEnabled
+        {
+            get => _isEnabled;
+            internal set => SetAndNotify(ref _isEnabled, value);
+        }
+
+        /// <summary>
+        ///     Gets the exception thrown while loading
+        /// </summary>
+        public Exception? LoadException
+        {
+            get => _loadException;
+            internal set => SetAndNotify(ref _loadException, value);
+        }
+
+        internal PluginImplementationEntity? Entity { get; set; }
 
         /// <summary>
         ///     Called when the implementation is activated
@@ -35,62 +52,75 @@ namespace Artemis.Core
 
         internal void SetEnabled(bool enable, bool isAutoEnable = false)
         {
-            if (enable && !IsEnabled)
+            if (enable == IsEnabled)
+                return;
+
+            if (Plugin == null)
+                throw new ArtemisCoreException("Cannot enable a plugin implementation that is not associated with a plugin");
+
+            lock (Plugin)
             {
+                if (!Plugin.IsEnabled)
+                    throw new ArtemisCoreException("Cannot enable a plugin implementation of a disabled plugin");
+
+                if (!enable)
+                {
+                    IsEnabled = false;
+
+                    // Even if disable failed, still leave it in a disabled state to avoid more issues
+                    InternalDisable();
+                    OnDisabled();
+                    return;
+                }
+                
                 try
                 {
-                    if (isAutoEnable && PluginInfo.GetLockFileCreated())
+                    if (isAutoEnable && GetLockFileCreated())
                     {
                         // Don't wrap existing lock exceptions, simply rethrow them
-                        if (PluginInfo.LoadException is ArtemisPluginLockException)
-                            throw PluginInfo.LoadException;
+                        if (LoadException is ArtemisPluginLockException)
+                            throw LoadException;
 
-                        throw new ArtemisPluginLockException(PluginInfo.LoadException);
+                        throw new ArtemisPluginLockException(LoadException);
                     }
 
                     IsEnabled = true;
-                    PluginInfo.IsEnabled = true;
-                    PluginInfo.CreateLockFile();
+                    CreateLockFile();
 
                     // Allow up to 15 seconds for plugins to activate.
                     // This means plugins that need more time should do their long running tasks in a background thread, which is intentional
-                    // Little meh: Running this from a different thread could cause deadlocks
-                    Task enableTask = Task.Run(InternalEnable);
-                    if (!enableTask.Wait(TimeSpan.FromSeconds(15)))
-                        throw new ArtemisPluginException(PluginInfo, "Plugin load timeout");
+                    ManualResetEvent wait = new ManualResetEvent(false);
+                    Thread work = new Thread(() =>
+                    {
+                        InternalEnable();
+                        wait.Set();
+                    });
+                    work.Start();
+                    wait.WaitOne(TimeSpan.FromSeconds(15));
+                    if (work.IsAlive)
+                    {
+                        work.Abort();
+                        throw new ArtemisPluginException(Plugin, "Plugin load timeout");
+                    }
 
-                    PluginInfo.LoadException = null;
+                    LoadException = null;
                     OnEnabled();
                 }
                 // If enable failed, put it back in a disabled state
                 catch (Exception e)
                 {
                     IsEnabled = false;
-                    PluginInfo.IsEnabled = false;
-                    PluginInfo.LoadException = e;
+                    LoadException = e;
                     throw;
                 }
                 finally
                 {
-                    if (!(PluginInfo.LoadException is ArtemisPluginLockException))
-                        PluginInfo.DeleteLockFile();
+                    // Clean up the lock file unless the failure was due to the lock file
+                    // After all, we failed but not miserably :)
+                    if (!(LoadException is ArtemisPluginLockException))
+                        DeleteLockFile();
                 }
             }
-            else if (!enable && IsEnabled)
-            {
-                IsEnabled = false;
-                PluginInfo.IsEnabled = false;
-
-                // Even if disable failed, still leave it in a disabled state to avoid more issues
-                InternalDisable();
-                OnDisabled();
-            }
-            // A failed load is still enabled in plugin info (to avoid disabling it permanently after a fail)
-            // update even that when manually disabling
-            else if (!enable && !IsEnabled)
-            {
-                PluginInfo.IsEnabled = false;
-            }
         }
 
         internal virtual void InternalEnable()
@@ -109,6 +139,35 @@ namespace Artemis.Core
             Disable();
         }
 
+        #region Loading
+
+        internal void CreateLockFile()
+        {
+            if (Plugin == null)
+                throw new ArtemisCoreException("Cannot lock a plugin implementation that is not associated with a plugin");
+
+            File.Create(Plugin.ResolveRelativePath($"{GetType().FullName}.lock")).Close();
+        }
+
+        internal void DeleteLockFile()
+        {
+            if (Plugin == null)
+                throw new ArtemisCoreException("Cannot lock a plugin implementation that is not associated with a plugin");
+
+            if (GetLockFileCreated())
+                File.Delete(Plugin.ResolveRelativePath($"{GetType().FullName}.lock"));
+        }
+
+        internal bool GetLockFileCreated()
+        {
+            if (Plugin == null)
+                throw new ArtemisCoreException("Cannot lock a plugin implementation that is not associated with a plugin");
+
+            return File.Exists(Plugin.ResolveRelativePath($"{GetType().FullName}.lock"));
+        }
+
+        #endregion
+
         #region Events
 
         /// <summary>

src/Artemis.Core/Plugins/PluginInfo.cs

@@ -1,10 +1,5 @@
 using System;
-using System.IO;
-using System.Reflection;
-using Artemis.Storage.Entities.Plugins;
-using McMaster.NETCore.Plugins;
 using Newtonsoft.Json;
-using Ninject;
 using Stylet;
 
 namespace Artemis.Core
@@ -16,14 +11,11 @@ namespace Artemis.Core
     public class PluginInfo : PropertyChangedBase
     {
         private string _description;
-        private DirectoryInfo _directory;
         private Guid _guid;
         private string _icon;
-        private Plugin _instance;
-        private bool _isEnabled;
-        private Exception _loadException;
         private string _main;
         private string _name;
+        private Plugin _plugin;
         private Version _version;
 
         internal PluginInfo()
@@ -92,93 +84,19 @@ namespace Artemis.Core
             internal set => SetAndNotify(ref _main, value);
         }
 
-        /// <summary>
-        ///     The plugins root directory
-        /// </summary>
-        public DirectoryInfo Directory
-        {
-            get => _directory;
-            internal set => SetAndNotify(ref _directory, value);
-        }
-
         /// <summary>
         ///     Gets the plugin this info is associated with
         /// </summary>
-        public Plugin Instance
+        public Plugin Plugin
         {
-            get => _instance;
-            internal set => SetAndNotify(ref _instance, value);
+            get => _plugin;
+            internal set => SetAndNotify(ref _plugin, value);
         }
 
-        /// <summary>
-        ///     Indicates whether the user enabled the plugin or not
-        /// </summary>
-        public bool IsEnabled
-        {
-            get => _isEnabled;
-            internal set => SetAndNotify(ref _isEnabled, value);
-        }
-
-        /// <summary>
-        ///     Gets the exception thrown while loading
-        /// </summary>
-        public Exception LoadException
-        {
-            get => _loadException;
-            internal set => SetAndNotify(ref _loadException, value);
-        }
-
-        /// <summary>
-        ///     The assembly the plugin code lives in
-        /// </summary>
-        public Assembly Assembly { get; internal set; }
-
-        /// <summary>
-        ///     The Ninject kernel of the plugin
-        /// </summary>
-        public IKernel Kernel { get; internal set; }
-
-        /// <summary>
-        ///     The PluginLoader backing this plugin
-        /// </summary>
-        internal PluginLoader PluginLoader { get; set; }
-
-        /// <summary>
-        ///     The entity representing the plugin
-        /// </summary>
-        internal PluginEntity PluginEntity { get; set; }
-
         /// <inheritdoc />
         public override string ToString()
         {
             return $"{Name} v{Version} - {Guid}";
         }
-
-        public string? ResolveRelativePath(string path)
-        {
-            return path == null ? null : Path.Combine(Directory.FullName, path);
-        }
-
-        internal void ApplyToEntity()
-        {
-            PluginEntity.Id = Guid;
-            PluginEntity.IsEnabled = IsEnabled;
-        }
-
-        internal void CreateLockFile()
-        {
-            File.Create(Path.Combine(Directory.FullName, "artemis.lock")).Close();
-        }
-
-        internal void DeleteLockFile()
-        {
-            if (GetLockFileCreated())
-                File.Delete(Path.Combine(Directory.FullName, "artemis.lock"));
-        }
-
-        internal bool GetLockFileCreated()
-        {
-            return File.Exists(Path.Combine(Directory.FullName, "artemis.lock"));
-        }
     }
 }

src/Artemis.Core/Plugins/TimedUpdateRegistration.cs

@@ -19,9 +19,9 @@ namespace Artemis.Core
             Interval = interval;
             Action = action;
 
-            PluginInfo.Instance.Enabled += InstanceOnEnabled;
-            PluginInfo.Instance.Disabled += InstanceOnDisabled;
-            if (PluginInfo.Instance.IsEnabled)
+            PluginInfo.Plugin.Enabled += InstanceOnEnabled;
+            PluginInfo.Plugin.Disabled += InstanceOnDisabled;
+            if (PluginInfo.Plugin.IsEnabled)
                 Start();
         }
 
@@ -31,9 +31,9 @@ namespace Artemis.Core
             Interval = interval;
             AsyncAction = asyncAction;
             
-            PluginInfo.Instance.Enabled += InstanceOnEnabled;
-            PluginInfo.Instance.Disabled += InstanceOnDisabled;
-            if (PluginInfo.Instance.IsEnabled)
+            PluginInfo.Plugin.Enabled += InstanceOnEnabled;
+            PluginInfo.Plugin.Disabled += InstanceOnDisabled;
+            if (PluginInfo.Plugin.IsEnabled)
                 Start();
         }
 
@@ -66,7 +66,7 @@ namespace Artemis.Core
         {
             lock (this)
             {
-                if (!PluginInfo.Instance.IsEnabled)
+                if (!PluginInfo.Plugin.IsEnabled)
                     throw new ArtemisPluginException("Cannot start a timed update for a disabled plugin");
 
                 if (_timer != null)
@@ -99,7 +99,7 @@ namespace Artemis.Core
 
         private void TimerOnElapsed(object sender, ElapsedEventArgs e)
         {
-            if (!PluginInfo.Instance.IsEnabled)
+            if (!PluginInfo.Plugin.IsEnabled)
                 return;
 
             lock (this)
@@ -108,7 +108,7 @@ namespace Artemis.Core
                 _lastEvent = DateTime.Now;
 
                 // Modules don't always want to update, honor that
-                if (PluginInfo.Instance is Module module && !module.IsUpdateAllowed)
+                if (PluginInfo.Plugin is Module module && !module.IsUpdateAllowed)
                     return;
 
                 if (Action != null)