massive cleanup of bot shutdown, fix #729, fix #993

bot.py

@@ -64,14 +64,12 @@ def __init__(self, botconfig):
 
         if botconfig['feature'].getboolean('server'):
             self.server = server.init_server(self)
-        # properly log quits.
-        self.connection.add_global_handler("quit", self.handle_quit, -21)
         # fix unicode problems
         self.connection.buffer_class.errors = 'replace'
 
     def handle_event(self, c, e):
-        handled_types = ['903', 'action', 'authenticate', 'bannedfromchan', 'cap', 'ctcpreply', 'disconnect', 'error', 'join', 'kick',
-                         'mode', 'nicknameinuse', 'nick', 'part', 'privmsg', 'privnotice', 'pubmsg', 'welcome']
+        handled_types = ['903', 'action', 'authenticate', 'bannedfromchan', 'cap', 'ctcpreply', 'error', 'join', 'kick',
+                         'mode', 'nicknameinuse', 'nick', 'part', 'privmsg', 'privnotice', 'pubmsg', 'quit', 'welcome']
         # We only need to do stuff for a sub-set of events.
         if e.type not in handled_types:
             return
@@ -101,30 +99,22 @@ def get_target(e):
         else:
             return e.source.nick
 
-    def handle_quit(self, _, e):
-        """Log quits."""
-        for channel in misc.get_channels(self.channels, e.source.nick):
-            self.handler.do_log(channel, e.source, e.arguments[0], 'quit')
-
-    def kill(self):
-        """ forcibly kills everything """
-        if hasattr(self, 'handler'):
-            self.handler.kill()
-        self.shutdown_server()
-
     def shutdown_server(self):
         if hasattr(self, 'server'):
             self.server.socket.close()
             self.server.shutdown()
 
-    def shutdown_workers(self):
+    def shutdown_workers(self, clean):
         if hasattr(self, 'handler'):
-            self.handler.workers.stop_workers()
+            self.handler.workers.stop_workers(clean)
+
+    def shutdown_mp(self, clean=True):
+        """ Shutdown all the multiprocessing.
 
-    def shutdown_mp(self):
-        """ Shutdown all the multiprocessing that we know about."""
+        :param bool clean: Whether to shutdown things cleanly, or force a quick and dirty shutdown.
+        """
         self.shutdown_server()
-        self.shutdown_workers()
+        self.shutdown_workers(clean)
 
     def do_rejoin(self, c, e):
         if e.arguments[0] in self.channels:
@@ -176,10 +166,15 @@ def handle_msg(self, c, e):
                 misc.ping(c, e, time.time())
         elif e.type == 'error':
             logging.error(e.target)
-        elif e.type == 'disconnect':
-            # Don't kill everything if we just ping timed-out
-            if e.arguments[0] == 'Goodbye, Cruel World!':
+        elif e.type == 'quit':
+            # Log quits.
+            for channel in misc.get_channels(self.channels, e.source.nick):
+                self.handler.do_log(channel, e.source, e.arguments[0], 'quit')
+            # If we're the one quiting, shut things down cleanly.
+            if e.source.nick == self.connection.real_nickname:
+                # FIXME: If this hangs or takes more then 5 seconds, we'll just reconnect.
                 self.shutdown_mp()
+                sys.exit(0)
         else:
             raise Exception('Un-handled event type %s' % e.type)
 
@@ -214,15 +209,11 @@ def reload_handler(self, c, e):
                 self.reload_event.clear()
 
 
-def main(args):
+def main():
     """The bot's main entry point.
 
-    | Setup logging.
-    | When troubleshooting startup, it may help to change the INFO to DEBUG.
     | Initialize the bot and start processing messages.
     """
-    loglevel = logging.DEBUG if args.debug else logging.INFO
-    logging.basicConfig(level=loglevel)
     config_file = path.join(path.dirname(__file__), 'config.cfg')
     if not path.exists(config_file):
         logging.info("Setting up config file")
@@ -235,11 +226,11 @@ def main(args):
     try:
         bot.start()
     except KeyboardInterrupt:
-        # keyboard interrupt means someone tried to ^C, shut down the bot
-        bot.kill()
+        # KeyboardInterrupt means someone tried to ^C, so shut down the bot
+        bot.shutdown_mp()
         sys.exit(0)
     except Exception as e:
-        bot.kill()
+        bot.shutdown_mp(False)
         logging.error("The bot died! %s" % e)
         output = "".join(traceback.format_exc())
         for line in output.split('\n'):
@@ -250,5 +241,7 @@ def main(args):
     multiprocessing.set_start_method('spawn')
     parser = argparse.ArgumentParser()
     parser.add_argument('-d', '--debug', help='Enable debug logging.', action='store_true')
-    parser_args = parser.parse_args()
-    main(parser_args)
+    args = parser.parse_args()
+    loglevel = logging.DEBUG if args.debug else logging.INFO
+    logging.basicConfig(level=loglevel)
+    main()

commands/quit.py

@@ -22,4 +22,5 @@ def cmd(send, msg, args):
     """Makes the bot disconnect and shut off
     Syntax: {command}
     """
-    args['handler'].shutdown()
+    # We can't actually shutdown the bot here, because this command is executing on one of the worker threads.
+    args['handler'].connection.quit('Goodbye, Cruel World!')

helpers/handler.py

@@ -339,12 +339,6 @@ def do_args(self, modargs, send, nick, target, source, name, msgtype):
                 raise Exception("Invalid Argument: %s" % arg)
         return realargs
 
-    def shutdown(self):
-        """ Cleanly shut ourself down """
-        self.send(self.config['core']['ctrlchan'], self.connection.real_nickname, 'Please ^C me, I won\'t die all the way =(', 'privmsg')
-        self.connection.disconnect('Goodbye, Cruel World!')
-        self.workers.stop_workers()
-
     def do_welcome(self, bot):
         """Do setup when connected to server.
 
@@ -363,10 +357,6 @@ def do_welcome(self, bot):
             for i in range(len(extrachans)):
                 self.workers.defer(i, False, self.connection.join, extrachans[i])
 
-    def kill(self):
-        """ Forcibly kill ourself """
-        self.workers.kill_workers()
-
     def is_ignored(self, nick):
         return nick in self.ignored
 

helpers/workers.py

@@ -15,10 +15,10 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
 import re
+import signal
 import threading
 import multiprocessing
 import concurrent.futures
-import logging
 from collections import namedtuple
 from threading import Timer
 from . import backtrace, babble, control
@@ -35,7 +35,7 @@ class Workers():
 
     def __init__(self, handler):
         with worker_lock:
-            self.pool = multiprocessing.Pool()
+            self.pool = multiprocessing.Pool(initializer=self.pool_init)
             self.events = {}
         with executor_lock:
             self.executor = concurrent.futures.ThreadPoolExecutor(4)
@@ -47,6 +47,10 @@ def send(msg, target=handler.config['core']['ctrlchan']):
         self.defer(3600, False, self.handle_pending, handler, send)
         self.defer(3600, False, self.check_babble, handler, send)
 
+    def pool_init(_):
+        """We ignore Ctrl-C in the poll workers, so that we can clean things up properly."""
+        signal.signal(signal.SIGINT, signal.SIG_IGN)
+
     def start_thread(self, func, *args, **kwargs):
         with executor_lock:
             self.executor.submit(func, *args, **kwargs)
@@ -60,7 +64,7 @@ def restart_pool(self):
         with worker_lock:
             self.pool.terminate()
             self.pool.join()
-            self.pool = multiprocessing.Pool()
+            self.pool = multiprocessing.Pool(initializer=self.pool_init)
 
     def run_action(self, func, args):
         try:
@@ -87,29 +91,16 @@ def cancel(self, eventid):
                 self.events[eventid].event.function(**self.events[eventid].event.kwargs)
             del self.events[eventid]
 
-    def stop_workers(self):
-        """ Cleanly stop workers and deferred events """
+    def stop_workers(self, clean):
+        """ Stop workers and deferred events """
         with executor_lock:
-            self.executor.shutdown(True)
+            self.executor.shutdown(clean)
             del self.executor
         with worker_lock:
-            self.pool.close()
-            self.pool.join()
-            del self.pool
-            for x in self.events.values():
-                x.event.cancel()
-            self.events.clear()
-
-    def kill_workers(self):
-        """ Forcibly kill all worker threads """
-        with executor_lock:
-            logging.info("Forcibly shutting down executor")
-            self.executor.shutdown(False)
-            del self.executor
-        with worker_lock:
-            logging.info("Killing pool")
-            self.pool.terminate()
-            logging.info("Joining pool")
+            if clean:
+                self.pool.close()
+            else:
+                self.pool.terminate()
             self.pool.join()
             del self.pool
             for x in self.events.values():

scripts/test.py

@@ -48,8 +48,7 @@ def setUpClass(cls):
 
     @classmethod
     def tearDownClass(cls):
-        cls.bot.shutdown_server()
-        cls.bot.shutdown_workers()
+        cls.bot.shutdown_mp()
 
     @classmethod
     def setup_handler(cls):
@@ -67,8 +66,7 @@ def config_mock(cls, section, option):
 
     def restart_workers(self):
         """Force all the workers to restart so we get the log message."""
-        self.bot.shutdown_server()
-        self.bot.shutdown_workers()
+        self.bot.shutdown_mp()
         self.bot.handler.workers.__init__(self.bot.handler)
         server_mod = importlib.import_module('helpers.server')
         self.bot.server = server_mod.init_server(self.bot)