Fix graceful server shutdown (#44)

At some point, our server's graceful shutdown logic broke. This fixes a couple issues:

- If a diff is in progress while the server is shutting down, it's possible for the diff retry mechanism to fight with the server about shutting down or starting up the process pool for diffing.

- The SIGINT signal for ctrl+c is getting passed down to the worker processes in the diff pool, which did not used to happen. We now add explicit initialization code to the workers so they ignore the signal, since the main server process will handle it and tell the workers what to do.

Author: Mr0grog

docs/source/release-history.rst

@@ -7,6 +7,8 @@ In Development
 
 - Fixes an issue where the diffing server could reset the process pool that manages the actual diffs multiple times unnecessarily, leading to wasted memory and CPU. If you are tracking logs and errors, this will also make error messages about the diffing server clearer — you’ll see “BrokenProcessPool” instead of “'NoneType' object does not support item assignment.” (`#38 <https://github.com/edgi-govdata-archiving/web-monitoring-diff/issues/38>`_)
 
+- Ensures the server shuts down gracefully when pressing ctrl+c or sending a SIGINT signal. (`#44 <https://github.com/edgi-govdata-archiving/web-monitoring-diff/issues/44>`_)
+
 - Fixes :func:`web_monitoring_diff.html_diff_render` to make sure the spacing of text and tags in the HTML source code of the diff matches the original. This resolves display issues on pages where CSS is used to treat spacing as significant. (`#36 <https://github.com/edgi-govdata-archiving/web-monitoring-diff/issues/36>`_)
 
 - Improve handling of lazy-loaded images in :func:`web_monitoring_diff.html_diff_render`. When images are lazy-loaded via JS, they usually use the ``data-src`` or ``data-srcset`` attributes, and we now check those, too. Additionally, if two images have no detectable URLs, we now treat them as the same, rather than different. (`#37 <https://github.com/edgi-govdata-archiving/web-monitoring-diff/issues/37>`_)

web_monitoring_diff/server/server.py

@@ -202,6 +202,10 @@ def _get_content_type_headers_from_url(url):
     os.environ.get('ACCESS_CONTROL_ALLOW_ORIGIN_HEADER')
 
 
+def initialize_diff_worker():
+    signal.signal(signal.SIGINT, signal.SIG_IGN)
+
+
 class DiffServer(tornado.web.Application):
     terminating = False
 
@@ -486,6 +490,10 @@ async def fetch_diffable_content(self, url, expected_hash, query_params):
 
         return response
 
+    # TODO: we should split out all the management of the executor and diffing
+    # (so this, get_diff_executor, caller, etc.) into a separate object owned
+    # by the server so we don't need weird bits checking the server's
+    # `terminating` state and so that all the parts are grouped together.
     async def diff(self, func, a, b, params, tries=2):
         """
         Actually do a diff between two pieces of content, optionally retrying
@@ -512,6 +520,9 @@ async def diff(self, func, a, b, params, tries=2):
     # NOTE: this doesn't do anything async, but if we change it to do so, we
     # need to add a lock (either asyncio.Lock or tornado.locks.Lock).
     def get_diff_executor(self, reset=False):
+        if self.application.terminating:
+            raise RuntimeError('Diff executor is being shut down.')
+
         executor = self.settings.get('diff_executor')
         if reset or not executor:
             if executor:
@@ -522,7 +533,8 @@ def get_diff_executor(self, reset=False):
                 except Exception:
                     pass
             executor = concurrent.futures.ProcessPoolExecutor(
-                DIFFER_PARALLELISM)
+                DIFFER_PARALLELISM,
+                initializer=initialize_diff_worker)
             self.settings['diff_executor'] = executor
 
         return executor