Merge pull request #54 from odin-detector/clean_shutdown

Improve odin-control shutdown and reload

src/odin/http/server.py

@@ -71,18 +71,20 @@ def __init__(self, config):
 
         # If HTTP is enabled, configure the application to listen on the specified address and
         # port
+        self.http_server = None
         if config.enable_http:
             try:
-                self.application.listen(config.http_port, config.http_addr)
+                self.http_server = self.application.listen(config.http_port, config.http_addr)
                 logging.info('HTTP server listening on %s:%s', config.http_addr, config.http_port)
             except OSError as listen_err:
                 logging.error(
                     "Failed to create HTTP server on %s:%s: %s", 
-                    config.http_addr, config.https_port, str(listen_err)
+                    config.http_addr, config.http_port, str(listen_err)
                 )
 
         # If HTTPS is enabled, create a SSL context and load the specified certificate and key,
         # then configure the application to listen on the specified address and port
+        self.https_server = None
         if config.enable_https:
             try:
                 ssl_ctx = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
@@ -91,7 +93,7 @@ def __init__(self, config):
                 logging.error("Failed to create SSL context for HTTPS: %s", str(ctx_err))
             else:
                 try:
-                    self.application.listen(
+                    self.https_server = self.application.listen(
                         config.https_port, config.http_addr, ssl_options=ssl_ctx
                     )
                     logging.info(
@@ -103,6 +105,19 @@ def __init__(self, config):
                         config.http_addr, config.https_port, str(listen_err)
                     )
 
+    def stop(self):
+        """Stop the HTTP/S server(s).
+
+        This method stops the underlying HTTP/S server(s) cleanly, preventing any new connections
+        from being accepted.
+        """
+        if self.http_server:
+            logging.debug("Stopping HTTP server")
+            self.http_server.stop()
+        if self.https_server:
+            logging.debug("Stopping HTTPS server")
+            self.https_server.stop()
+
     def log_request(self, handler):
         """Log completed request information.
 

src/odin/main.py

@@ -11,17 +11,13 @@
 import threading
 
 import tornado.ioloop
+from tornado.autoreload import add_reload_hook
 
 from odin.http.server import HttpServer
 from odin.config.parser import ConfigParser, ConfigError
 from odin.logconfig import add_graylog_handler
 
-
-def shutdown_handler():  # pragma: no cover
-    """Handle interrupt signals gracefully and shutdown IOLoop."""
-    logging.info('Interrupt signal received, shutting down')
-    tornado.ioloop.IOLoop.instance().stop()
-
+_stop_ioloop = False  # Global variable to indicate ioloop should be shut down
 
 def main(argv=None):
     """Run the odin-control server.
@@ -68,15 +64,66 @@ def main(argv=None):
             config.graylog_static_fields
         )
 
+     # Get the Tornado ioloop instance
+    ioloop = tornado.ioloop.IOLoop.instance()
+
     # Launch the HTTP server with the parsed configuration
     http_server = HttpServer(config)
 
-    # Register a SIGINT signal handler only if this is the main thread
+    # If debug mode is enabled, add an autoreload hook to the server to ensure that adapter cleanup
+    # methods are called as the server reloads
+    if config.debug_mode:
+        add_reload_hook(http_server.cleanup_adapters)
+
+    def shutdown_handler(sig_name):  # pragma: no cover
+        """Shut down the running server cleanly.
+
+        This inner function implements a signal handler to shut down the running server cleanly in
+        response to a signal. The underlying HTTP server is stopped, preventing new connections
+        from being accepted, and a global flag set true to allow a periodic task to terminate the
+        ioloop cleanly.
+
+        :param signum: signal number that the handler was invoked with
+        :param _: unused stack frame
+        """
+        global _stop_ioloop
+        logging.info('%s signal received, shutting down', sig_name)
+
+        # Stop the HTTP server
+        http_server.stop()
+
+        # Tell the periodic callback to stop the ioloop when next invokved
+        _stop_ioloop = True
+
+    def stop_ioloop():  # pragma: no cover
+        """Stop the running ioloop cleanly.
+
+        This inner function is run as a periodic callback and stops the ioloop when requested by
+        the signal handler. This mechansim is necessary to ensure that the ioloop stops cleanly
+        under all conditions, for instance when adapters and handlers have not been correctly
+        initialised.
+        """
+        global _stop_ioloop
+        if _stop_ioloop:
+             logging.debug("Stopping ioloop")
+
+             # Stop the ioloop
+             ioloop.stop()
+
+    # Register a shutdown signal handler and start an ioloop stop callback only if this is the
+    # main thread
     if isinstance(threading.current_thread(), threading._MainThread):  # pragma: no cover
-        signal.signal(signal.SIGINT, lambda signum, frame: shutdown_handler())
+        signal.signal(signal.SIGINT, lambda signum, frame: shutdown_handler('Interrupt'))
+        signal.signal(signal.SIGTERM, lambda signum, frame: shutdown_handler('Terminate'))
+        tornado.ioloop.PeriodicCallback(stop_ioloop, 1000).start()
+
+    # Start the ioloop
+    ioloop.start()
 
-    # Enter IO processing loop
-    tornado.ioloop.IOLoop.instance().start()
+    # If the application isn't shutting down due to the signal handler being invoked (e.g. when
+    # running in a secondary thread), ensure the HTTP server stops cleanly
+    if not _stop_ioloop:
+        http_server.stop()
 
     # At shutdown, clean up the state of the loaded adapters
     http_server.cleanup_adapters()

tests/test_server.py

@@ -215,6 +215,7 @@ def test_bad_access_log_level(self, server_config, caplog):
         bad_level  = 'wibble'
         server_config.access_logging = bad_level
         http_server = HttpServer(server_config)
+        http_server.stop()
 
         assert log_message_seen(caplog, logging.ERROR,
             'Access logging level {} not recognised'.format(bad_level))
@@ -235,6 +236,21 @@ def test_server_missing_adapters(self, no_adapter_server, caplog):
             'Failed to resolve API adapters: No adapters specified in configuration',
             when="setup")
 
+class TestOdinServerListenFailed(object):
+
+    def test_http_server_listen_fails(self, caplog):
+
+        config_1 = ServerConfig()
+        config_2 = ServerConfig()
+
+        server1 = HttpServer(config_1)
+        server2 = HttpServer(config_2)
+
+        server1.stop()
+        server2.stop()
+
+        assert log_message_seen(caplog, logging.ERROR, "Address already in use")
+
 class MockHandler(object):
     """Class for mocking tornado request handler objects."""
 
@@ -291,6 +307,9 @@ def do_log_request(self, http_status, level):
                 msg_seen = True
         return msg_seen
 
+    def __del__(self):
+        self.http_server.stop()
+
 @pytest.fixture()
 def logging_test_server(caplog):
     """
@@ -321,8 +340,11 @@ def __init__(self, caplog):
 
         self.server_config = ServerConfig()
         self.server_config.enable_https = True
-        self.https_Server = HttpServer(self.server_config)
-        self.caplog = caplog   
+        self.https_server = HttpServer(self.server_config)
+        self.caplog = caplog
+
+    def __del__(self):
+        self.https_server.stop()
 
 @pytest.fixture()
 def https_test_server(caplog):
@@ -346,6 +368,7 @@ def test_https_valid_config(self, server_config, ssl_test_cert, caplog):
         server_config.ssl_cert_file = ssl_test_cert.cert_file 
         server_config.ssl_key_file = ssl_test_cert.key_file 
         server = HttpServer(server_config)
+        server.stop()
 
         assert log_message_seen(
             caplog, logging.INFO,
@@ -357,6 +380,7 @@ def test_https_valid_config(self, server_config, ssl_test_cert, caplog):
     def test_https_no_ssl_files(self, server_config, caplog):
         server_config.enable_https = True
         server = HttpServer(server_config)
+        server.stop()
         assert log_message_seen(
             caplog, logging.ERROR,
             "Failed to create SSL context for HTTPS: [Errno 2] No such file or directory",
@@ -369,6 +393,7 @@ def test_https_listen_error(self, server_config, ssl_test_cert, caplog):
         server_config.ssl_key_file = ssl_test_cert.key_file
         server_config.https_port = 443
         server = HttpServer(server_config)
+        server.stop()
 
         assert log_message_seen(
             caplog, logging.ERROR,

tox.ini

@@ -17,7 +17,7 @@ deps =
     pytest-cov
     requests
     py27: mock
-    py{36,37,38,39}: pytest-asyncio
+    py{36,37,38,39}: pytest-asyncio<0.22
     tornado4: tornado>=4.0,<5.0
     tornado5: tornado>=5.0,<6.0
     tornado6: tornado>=6.0