Issue: #36

Fixed a long-standing issue where if an exception happened while loading plugins, the bot would hang. Turns out, we weren't cleaning up some things properly. Specifically, EventScheduler. This has been resolved. * Added new Regression test to ensure the process exits if an assembly is missing. * We throw an exception if an assembly fails to load or init. Its just easier that way. * EventSchedule does not start until the IRC Connection is started. * Updated Regression Test Fixtures such that it is able to not wait for Chaskis to connect.
xforever1313 · Nov 29, 2020 · 39829ba · 39829ba
1 parent d6081d0
commit 39829ba
Show file tree

Hide file tree

Showing 14 changed files with 249 additions and 125 deletions.
diff --git a/Chaskis/Chaskis/Chaskis.cs b/Chaskis/Chaskis/Chaskis.cs
@@ -119,23 +119,21 @@ public void InitState1_LoadIrcConfig()
         /// Loads the Plugins from the chaskis root.
         /// The IRC config MUST be loaded first.
         /// </summary>
-        /// <returns>True if load was successful, else false.</returns>
-        public bool InitStage2_LoadPlugins()
+        public void InitStage2_LoadPlugins()
         {
             string pluginConfigFile = Path.Combine( this.chaskisRoot, "PluginConfig.xml" );
 
             StaticLogger.Log.WriteLine( "Using Plugin config file '{0}'", pluginConfigFile );
 
             IList<AssemblyConfig> pluginList = XmlLoader.ParsePluginConfig( pluginConfigFile );
-            return InitStage2_LoadPlugins( pluginList );
+            InitStage2_LoadPlugins( pluginList );
         }
 
         /// <summary>
         /// Loads plugins via a plugin list.
         /// </summary>
         /// <param name="pluginList">The list of plugins to load.</param>
-        /// <returns>True if load was successful, else false.</returns>
-        public bool InitStage2_LoadPlugins( IList<AssemblyConfig> pluginList )
+        public void InitStage2_LoadPlugins( IList<AssemblyConfig> pluginList )
         {
             if( this.ircConfig == null )
             {
@@ -154,26 +152,19 @@ public bool InitStage2_LoadPlugins( IList<AssemblyConfig> pluginList )
                 this.defaultHandlers,
                 new GenericLogger()
             );
-
-
-            if( manager.LoadPlugins(
-                    pluginList,
-                    new List<PluginConfig>() { defaultChaskisPlugin },
-                    this.ircConfig,
-                    this.ircBot.Scheduler,
-                    this.ircBot.ChaskisEventSender,
-                    this.httpClient,
-                    this.chaskisRoot
-                )
-            )
-            {
-                this.plugins = manager.Plugins;
-                return true;
-            }
-            else
-            {
-                return false;
-            }
+
+
+            manager.LoadPlugins(
+                pluginList,
+                new List<PluginConfig>() { defaultChaskisPlugin },
+                this.ircConfig,
+                this.ircBot.Scheduler,
+                this.ircBot.ChaskisEventSender,
+                this.httpClient,
+                this.chaskisRoot
+            );
+
+            this.plugins = manager.Plugins;
         }
 
         /// <summary>

diff --git a/Chaskis/Chaskis/ChaskisMain.cs b/Chaskis/Chaskis/ChaskisMain.cs
@@ -57,11 +57,7 @@ public async Task RunChaskis( IReadOnlyList<string> args, CancellationToken canc
                     using( Chaskis chaskis = new Chaskis( parser.ChaskisRoot ) )
                     {
                         chaskis.InitState1_LoadIrcConfig();
-                        bool pluginLoaded = chaskis.InitStage2_LoadPlugins();
-                        if( ( pluginLoaded == false ) )
-                        {
-                            throw new PluginLoadException( "Fail on assembly was enabled.  Terminating." );
-                        }
+                        chaskis.InitStage2_LoadPlugins();
                         chaskis.InitStage3_DefaultHandlers();
                         chaskis.InitStage4_OpenConnection();
 

diff --git a/Chaskis/Chaskis/PluginManager.cs b/Chaskis/Chaskis/PluginManager.cs
@@ -62,7 +62,7 @@ public PluginManager()
         /// <param name="existingPlugins">Already created plugins that do not need to be inited via reflection.</param>
         /// <param name="ircConfig">The irc config we are using.</param>
         /// <param name="chaskisConfigRoot">The root of the chaskis config.</param>
-        public bool LoadPlugins(
+        public void LoadPlugins(
             IList<AssemblyConfig> assemblyList,
             IList<PluginConfig> existingPlugins,
             IIrcConfig ircConfig,
@@ -72,7 +72,7 @@ public bool LoadPlugins(
             string chaskisConfigRoot
         )
         {
-            bool success = true;
+            List<Exception> exceptions = new List<Exception>();
 
             foreach( AssemblyConfig assemblyConfig in assemblyList )
             {
@@ -121,7 +121,7 @@ where type.IsDefined( typeof( ChaskisPlugin ), false )
                 {
                     StringBuilder errorString = new StringBuilder();
                     errorString.AppendLine( "*************" );
-                    errorString.AppendLine( "Warning! Error when loading assembly " + assemblyConfig.AssemblyPath + ":" );
+                    errorString.AppendLine( "Error when loading assembly " + assemblyConfig.AssemblyPath + ":" );
                     errorString.AppendLine( e.Message );
                     errorString.AppendLine();
                     errorString.AppendLine( e.StackTrace );
@@ -134,12 +134,20 @@ where type.IsDefined( typeof( ChaskisPlugin ), false )
                     }
                     errorString.AppendLine( "*************" );
 
-                    success = false;
-
                     StaticLogger.Log.ErrorWriteLine( errorString.ToString() );
+
+                    exceptions.Add( e );
                 }
             }
 
+            if( exceptions.Count != 0 )
+            {
+                throw new AggregateException(
+                    "Exceptions found when loading plugins:",
+                    exceptions
+                );
+            }
+
             foreach( PluginConfig existingPlugin in existingPlugins )
             {
                 this.plugins.Add( existingPlugin.Name, existingPlugin );
@@ -197,13 +205,19 @@ where type.IsDefined( typeof( ChaskisPlugin ), false )
                     }
                     errorString.AppendLine( "*************" );
 
-                    success = false;
-
                     StaticLogger.Log.ErrorWriteLine( errorString.ToString() );
+
+                    exceptions.Add( e );
                 }
             }
 
-            return success;
+            if( exceptions.Count != 0 )
+            {
+                throw new AggregateException(
+                    "Exceptions found when initializing plugins:",
+                    exceptions
+                );
+            }
         }
     }
 }
diff --git a/Chaskis/ChaskisCore/IConnection.cs b/Chaskis/ChaskisCore/IConnection.cs
@@ -5,12 +5,14 @@
 //          http://www.boost.org/LICENSE_1_0.txt)
 //
 
+using System;
+
 namespace Chaskis.Core
 {
     /// <summary>
     /// Interface to an IRC connection.
     /// </summary>
-    public interface IConnection : IIrcWriter
+    public interface IConnection : IIrcWriter, IDisposable
     {
         /// <summary>
         /// Returns the IRCConfig.
@@ -31,10 +33,5 @@ public interface IConnection : IIrcWriter
         /// Connects using the supplied settings.
         /// </summary>
         void Connect();
-
-        /// <summary>
-        /// Disconnects the connection.
-        /// </summary>
-        void Disconnect();
     }
 }
diff --git a/Chaskis/ChaskisCore/IrcBot.cs b/Chaskis/ChaskisCore/IrcBot.cs
@@ -106,9 +106,9 @@ public void Start()
         /// </summary>
         public void Dispose()
         {
-            if( this.ircConnection.IsConnected )
+            try
             {
-                try
+                if( this.ircConnection.IsConnected )
                 {
                     // Stop reacting to strings from the server, we don't care any more.
                     this.ircConnection.ReadEvent -= this.IrcConnection_ReadEvent;
@@ -120,10 +120,10 @@ public void Dispose()
 
                     this.parsingQueue.WaitForAllEventsToExecute();
                 }
-                finally
-                {
-                    this.ircConnection.Disconnect();
-                }
+            }
+            finally
+            {
+                this.ircConnection?.Dispose();
             }
         }
 

diff --git a/Chaskis/ChaskisCore/IrcConnection.cs b/Chaskis/ChaskisCore/IrcConnection.cs
@@ -200,6 +200,7 @@ public void Init()
             if( this.inited == false )
             {
                 // Start Executing
+                this.eventScheduler.Start();
                 this.writerQueue.Start();
                 this.inited = true;
             }
@@ -662,50 +663,6 @@ private void AddToWriterQueue( Action action )
             );
         }
 
-        /// <summary>
-        /// Disconnected.
-        /// No-Op if already disconnected.
-        /// </summary>
-        public void Disconnect()
-        {
-            if( IsConnected != false )
-            {
-                StaticLogger.Log.WriteLine( "Disconnecting..." );
-
-                // Stop all scheduled events.  This will prevent any more events
-                // from being queued from this perspective.
-                this.eventScheduler.Dispose();
-
-                // - Next, prevent the reader thread from adding more events.
-                //   Set KeepReading to false.  The abort logic in the Reader
-                //   thread depends on this.  This will also prevent any more
-                //   events being posted to the string parsing queue.
-                this.KeepReading = false;
-
-                // - We could be in the reconnecting state.  Trigger that thread to awaken
-                //   if its sleeping between reconnects.
-                //   Since KeepReading is set to false, the reconnection thread wil exit.
-                this.reconnectAbortEvent.Set();
-
-                // Drain our writer queue before disconnecting so everything that needs to go out goes out.
-                {
-                    ManualResetEvent doneEvent = new ManualResetEvent( false );
-                    this.writerQueue.AddEvent( () => doneEvent.Set() );
-
-                    // Wait for our last event to execute before leaving.
-                    doneEvent.WaitOne();
-                    this.writerQueue.Dispose();
-                }
-
-                this.DisconnectHelper();
-
-                this.watchDog.Dispose();
-                this.connection.Dispose();
-
-                StaticLogger.Log.WriteLine( "Disconnect Complete." );
-            }
-        }
-
         /// <summary>
         /// Helps disconnect the connection.
         /// </summary>
@@ -755,13 +712,49 @@ private void DisconnectHelper()
         }
 
         /// <summary>
-        /// Cleans up everything.
-        /// Calls Disconnect.
+        /// Disconnects and cleans up everything.
         /// </summary>
         public void Dispose()
         {
-            Disconnect();
+            // Stop all scheduled events.  This will prevent any more events
+            // from being queued from this perspective.  This should be called
+            // regardless if we are connected or not.
+            this.eventScheduler.Dispose();
+
+            if( IsConnected != false )
+            {
+                StaticLogger.Log.WriteLine( "Disconnecting..." );
+
+                // - Next, prevent the reader thread from adding more events.
+                //   Set KeepReading to false.  The abort logic in the Reader
+                //   thread depends on this.  This will also prevent any more
+                //   events being posted to the string parsing queue.
+                this.KeepReading = false;
+
+                // - We could be in the reconnecting state.  Trigger that thread to awaken
+                //   if its sleeping between reconnects.
+                //   Since KeepReading is set to false, the reconnection thread wil exit.
+                this.reconnectAbortEvent.Set();
+
+                // Drain our writer queue before disconnecting so everything that needs to go out goes out.
+                {
+                    ManualResetEvent doneEvent = new ManualResetEvent( false );
+                    this.writerQueue.AddEvent( () => doneEvent.Set() );
+
+                    // Wait for our last event to execute before leaving.
+                    doneEvent.WaitOne();
+                    this.writerQueue.Dispose(); // Dispose now so no more events get executed.
+                }
+
+                this.DisconnectHelper();
+
+                StaticLogger.Log.WriteLine( "Disconnect Complete." );
+            }
+
+            this.watchDog.Dispose();
+            this.connection.Dispose();
             this.writerQueue.OnError -= this.EventQueue_OnError;
+            this.writerQueue.Dispose();
         }
 
         /// <summary>

diff --git a/Chaskis/RegressionTests/Environments/MissingAssemblyEnvironment/IrcConfig.xml b/Chaskis/RegressionTests/Environments/MissingAssemblyEnvironment/IrcConfig.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<ircbotconfig>
+    <server>localhost</server>
+    <channels>
+        <channel>#chaskistest</channel>
+        <channel>#chaskistest2</channel>
+    </channels>
+    <port>{%port%}</port>
+    <username>chaskisbot</username>
+    <nick>chaskisbot</nick>
+    <realname>Chaskis Test Bot</realname>
+    <password></password>
+    <ratelimit>100</ratelimit>
+    <quitmessage>I'm going down!</quitmessage>
+    <admins>
+        <admin>adminuser</admin>
+    </admins>
+    <bridgebots>
+        <bridgebot>
+            <botname>xforever\d*</botname>
+            <botregex><![CDATA[^<(?<bridgeUser>\w+)>\s+(?<bridgeMessage>.+)]]></botregex>
+        </bridgebot>
+    </bridgebots>
+</ircbotconfig>
diff --git a/Chaskis/RegressionTests/Environments/MissingAssemblyEnvironment/PluginConfig.xml b/Chaskis/RegressionTests/Environments/MissingAssemblyEnvironment/PluginConfig.xml
@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<pluginconfig>
+    <assembly path="{%chaskispath%}/Plugins/ConsoleOut/ConsoleOut.dll" />
+    <assembly path="does_not_exist.dll" />
+</pluginconfig>
diff --git a/Chaskis/RegressionTests/TestFixtures/TestCore/ChaskisFixtureConfig.cs b/Chaskis/RegressionTests/TestFixtures/TestCore/ChaskisFixtureConfig.cs
@@ -25,7 +25,13 @@ public enum ConnectionWaitMode
         /// <summary>
         /// Wait until we finish joining all channels.
         /// </summary>
-        WaitForFinishJoiningChannels
+        WaitForFinishJoiningChannels,
+
+        /// <summary>
+        /// We should expect the server to NOT connect to anything,
+        /// as the process may purposefully not connect.
+        /// </summary>
+        ExpectNoConnection
     }
 
     public class ChaskisFixtureConfig

diff --git a/Chaskis/RegressionTests/TestFixtures/TestCore/ChaskisProcess.cs b/Chaskis/RegressionTests/TestFixtures/TestCore/ChaskisProcess.cs
@@ -174,6 +174,22 @@ public bool StopProcess( int timeout = TestConstants.DefaultTimeout )
             }
 
             this.process.StandardInput.WriteLine( "q" );
+            return this.Wait( timeout );
+        }
+
+        /// <summary>
+        /// Waits for the process to stop. This really should only be called
+        /// if we are expecting the process to exit on its own.
+        /// </summary>
+        /// <param name="timeout"></param>
+        /// <returns></returns>
+        public bool Wait( int timeout = TestConstants.DefaultTimeout )
+        {
+            if( this.process == null )
+            {
+                throw new InvalidOperationException( "There is no process to wait for!" );
+            }
+
             bool success = this.process.WaitForExit( timeout );
             if( success )
             {