From 15cdea59278923128a70656e3a3aa76f80183681 Mon Sep 17 00:00:00 2001 From: Peter Foley Date: Thu, 23 Apr 2015 14:48:53 -0400 Subject: [PATCH] massive cleanup of bot shutdown, fix #729, fix #993 --- bot.py | 59 ++++++++++++++++++++-------------------------- commands/quit.py | 3 ++- helpers/handler.py | 10 -------- helpers/workers.py | 37 +++++++++++------------------ scripts/test.py | 6 ++--- 5 files changed, 44 insertions(+), 71 deletions(-) diff --git a/bot.py b/bot.py index 56750ec6e..efac74881 100755 --- a/bot.py +++ b/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() diff --git a/commands/quit.py b/commands/quit.py index f7771783f..cf3b2951d 100644 --- a/commands/quit.py +++ b/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!') diff --git a/helpers/handler.py b/helpers/handler.py index 77c8b5136..996ef689f 100644 --- a/helpers/handler.py +++ b/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 diff --git a/helpers/workers.py b/helpers/workers.py index b1f4df995..cd5c717fa 100644 --- a/helpers/workers.py +++ b/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(): diff --git a/scripts/test.py b/scripts/test.py index c63eae330..5e623531d 100755 --- a/scripts/test.py +++ b/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)