tornado: Unfork tornado.autoreload.

We previously forked tornado.autoreload to work around a problem where
it would crash if you introduce a syntax error and not recover if you
fix it (https://github.com/tornadoweb/tornado/issues/2398).

A much more maintainable workaround for that issue, at least in
current Tornado, is to use tornado.autoreload as the main module.

Signed-off-by: Anders Kaseorg <anders@zulip.com>

tools/run-dev.py

@@ -84,8 +84,10 @@ if options.test:
     settings_module = "zproject.test_settings"
     # Don't auto-reload when running Puppeteer tests
     runserver_args = ["--noreload"]
+    tornado_autoreload = []
 else:
     settings_module = "zproject.settings"
+    tornado_autoreload = ["-m", "tornado.autoreload"]
 
 manage_args = [f"--settings={settings_module}"]
 os.environ["DJANGO_SETTINGS_MODULE"] = settings_module
@@ -134,6 +136,8 @@ def server_processes() -> List[List[str]]:
         [
             "env",
             "PYTHONUNBUFFERED=1",
+            "python3",
+            *tornado_autoreload,
             "./manage.py",
             "runtornado",
             *manage_args,

tools/test-backend

@@ -115,7 +115,6 @@ not_yet_fully_covered = [
     "zerver/lib/test_console_output.py",
     "zerver/openapi/python_examples.py",
     # Tornado should ideally have full coverage, but we're not there.
-    "zerver/tornado/autoreload.py",
     "zerver/tornado/descriptors.py",
     "zerver/tornado/django_api.py",
     "zerver/tornado/event_queue.py",

zerver/management/commands/runtornado.py

@@ -3,9 +3,10 @@ import sys
 from typing import Any, Callable
 from urllib.parse import SplitResult
 
+import __main__
 from django.conf import settings
 from django.core.management.base import BaseCommand, CommandError, CommandParser
-from tornado import ioloop
+from tornado import autoreload, ioloop
 from tornado.log import app_log
 
 # We must call zerver.tornado.ioloop_logging.instrument_tornado_ioloop
@@ -20,7 +21,6 @@ instrument_tornado_ioloop()
 
 from zerver.lib.debug import interactive_debug_listen
 from zerver.tornado.application import create_tornado_application, setup_tornado_rabbitmq
-from zerver.tornado.autoreload import start as zulip_autoreload_start
 from zerver.tornado.event_queue import (
     add_client_gc_hook,
     get_wrapped_process_notification,
@@ -93,8 +93,6 @@ class Command(BaseCommand):
             try:
                 # Application is an instance of Django's standard wsgi handler.
                 application = create_tornado_application()
-                if settings.AUTORELOAD:
-                    zulip_autoreload_start()
 
                 # start tornado web server in single-threaded mode
                 http_server = httpserver.HTTPServer(application, xheaders=True)
@@ -112,8 +110,17 @@ class Command(BaseCommand):
                 if django.conf.settings.DEBUG:
                     instance.set_blocking_log_threshold(5)
                     instance.handle_callback_exception = handle_callback_exception
+
+                if hasattr(__main__, "add_reload_hook"):
+                    autoreload.start()
+
                 instance.start()
             except KeyboardInterrupt:
+                # Monkey patch tornado.autoreload to prevent it from continuing
+                # to watch for changes after catching our SystemExit. Otherwise
+                # the user needs to press Ctrl+C twice.
+                __main__.wait = lambda: None
+
                 sys.exit(0)
 
         inner_run()

zerver/tornado/application.py

@@ -2,9 +2,9 @@ import atexit
 
 import tornado.web
 from django.conf import settings
+from tornado import autoreload
 
 from zerver.lib.queue import get_queue_client
-from zerver.tornado import autoreload
 from zerver.tornado.handlers import AsyncDjangoHandler
 
 

zerver/tornado/autoreload.py

@@ -1,240 +0,0 @@
-# mypy: ignore_errors
-
-# Copyright 2009 Facebook
-#
-# Licensed under the Apache License, Version 2.0 (the "License"); you may
-# not use this file except in compliance with the License. You may obtain
-# a copy of the License at
-#
-#     http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-# License for the specific language governing permissions and limitations
-# under the License.
-
-"""Automatically restart the server when a source file is modified.
-
-Most applications should not access this module directly.  Instead,
-pass the keyword argument ``autoreload=True`` to the
-`tornado.web.Application` constructor (or ``debug=True``, which
-enables this setting and several others).  This will enable autoreload
-mode as well as checking for changes to templates and static
-resources.  Note that restarting is a destructive operation and any
-requests in progress will be aborted when the process restarts.  (If
-you want to disable autoreload while using other debug-mode features,
-pass both ``debug=True`` and ``autoreload=False``).
-
-This module can also be used as a command-line wrapper around scripts
-such as unit test runners.  See the `main` method for details.
-
-The command-line wrapper and Application debug modes can be used together.
-This combination is encouraged as the wrapper catches syntax errors and
-other import-time failures, while debug mode catches changes once
-the server has started.
-
-This module depends on `.IOLoop`, so it will not work in WSGI applications
-and Google App Engine.  It also will not work correctly when `.HTTPServer`'s
-multi-process mode is used.
-
-Reloading loses any Python interpreter command-line arguments (e.g. ``-u``)
-because it re-executes Python using ``sys.executable`` and ``sys.argv``.
-Additionally, modifying these variables will cause reloading to behave
-incorrectly.
-
-"""
-
-# Further patched by Zulip check whether the code we're about to
-# reload actually imports before reloading into it.  This fixes a
-# major development workflow problem, where if one did a `git rebase`,
-# Tornado would crash itself by auto-reloading into a version of the
-# code that didn't work.
-
-
-import functools
-import importlib
-import os
-import subprocess
-import sys
-import traceback
-import types
-import weakref
-
-from tornado import ioloop, process
-from tornado.log import gen_log
-
-try:
-    import signal
-except ImportError:
-    signal = None
-
-# os.execv is broken on Windows and can't properly parse command line
-# arguments and executable name if they contain whitespaces. subprocess
-# fixes that behavior.
-_has_execv = sys.platform != "win32"
-
-_watched_files = set()
-_reload_hooks = []
-_reload_attempted = False
-_io_loops = weakref.WeakKeyDictionary()
-needs_to_reload = False
-
-
-def start(io_loop=None, check_time=500):
-    """Begins watching source files for changes.
-
-    .. versionchanged:: 4.1
-       The ``io_loop`` argument is deprecated.
-    """
-    io_loop = io_loop or ioloop.IOLoop.current()
-    if io_loop in _io_loops:
-        return
-    _io_loops[io_loop] = True
-    if len(_io_loops) > 1:
-        gen_log.warning("tornado.autoreload started more than once in the same process")
-    modify_times = {}
-    callback = functools.partial(_reload_on_update, modify_times)
-    scheduler = ioloop.PeriodicCallback(callback, check_time, io_loop=io_loop)
-    scheduler.start()
-
-
-def wait():
-    """Wait for a watched file to change, then restart the process.
-
-    Intended to be used at the end of scripts like unit test runners,
-    to run the tests again after any source file changes (but see also
-    the command-line interface in `main`)
-    """
-    io_loop = ioloop.IOLoop()
-    start(io_loop)
-    io_loop.start()
-
-
-def watch(filename):
-    """Add a file to the watch list.
-
-    All imported modules are watched by default.
-    """
-    _watched_files.add(filename)
-
-
-def add_reload_hook(fn):
-    """Add a function to be called before reloading the process.
-
-    Note that for open file and socket handles it is generally
-    preferable to set the ``FD_CLOEXEC`` flag (using `fcntl` or
-    ``tornado.platform.auto.set_close_exec``) instead
-    of using a reload hook to close them.
-    """
-    _reload_hooks.append(fn)
-
-
-def _reload_on_update(modify_times):
-    global needs_to_reload
-    if _reload_attempted:
-        # We already tried to reload and it didn't work, so don't try again.
-        return
-    if process.task_id() is not None:
-        # We're in a child process created by fork_processes.  If child
-        # processes restarted themselves, they'd all restart and then
-        # all call fork_processes again.
-        return
-    for module in list(sys.modules.values()):
-        # Some modules play games with sys.modules (e.g. email/__init__.py
-        # in the standard library), and occasionally this can cause strange
-        # failures in getattr.  Just ignore anything that's not an ordinary
-        # module.
-        if not isinstance(module, types.ModuleType):
-            continue
-        path = getattr(module, "__file__", None)
-        if not path:
-            continue
-        if path.endswith(".pyc") or path.endswith(".pyo"):
-            path = path[:-1]
-
-        result = _check_file(modify_times, module, path)
-        if result is False:
-            # If any files errored, we abort this attempt at reloading.
-            return
-        if result is True:
-            # If any files had actual changes that import properly,
-            # we'll plan to reload the next time we run with no files
-            # erroring.
-            needs_to_reload = True
-
-    if needs_to_reload:
-        _reload()
-
-
-def _check_file(modify_times, module, path):
-    try:
-        modified = os.stat(path).st_mtime
-    except Exception:
-        return
-    if path not in modify_times:
-        modify_times[path] = modified
-        return
-    if modify_times[path] != modified:
-        gen_log.info("%s modified; restarting server", path)
-        modify_times[path] = modified
-    else:
-        return
-
-    if path == __file__ or path == os.path.join(os.path.dirname(__file__), "event_queue.py"):
-        # Assume that the autoreload library itself imports correctly,
-        # because reloading this file will destroy its state,
-        # including _reload_hooks
-        return True
-
-    try:
-        importlib.reload(module)
-    except Exception:
-        gen_log.error(f"Error importing {path}, not reloading")
-        traceback.print_exc()
-        return False
-    return True
-
-
-def _reload():
-    global _reload_attempted
-    _reload_attempted = True
-    for fn in _reload_hooks:
-        fn()
-    # Make sure any output from reload hooks makes it to stdout.
-    sys.stdout.flush()
-    if hasattr(signal, "setitimer"):
-        # Clear the alarm signal set by
-        # ioloop.set_blocking_log_threshold so it doesn't fire
-        # after the exec.
-        signal.setitimer(signal.ITIMER_REAL, 0, 0)
-    # sys.path fixes: see comments at top of file.  If sys.path[0] is an empty
-    # string, we were (probably) invoked with -m and the effective path
-    # is about to change on re-exec.  Add the current directory to $PYTHONPATH
-    # to ensure that the new process sees the same path we did.
-    path_prefix = "." + os.pathsep
-    if sys.path[0] == "" and not os.environ.get("PYTHONPATH", "").startswith(path_prefix):
-        os.environ["PYTHONPATH"] = path_prefix + os.environ.get("PYTHONPATH", "")
-    if not _has_execv:
-        subprocess.Popen([sys.executable] + sys.argv)
-        sys.exit(0)
-    else:
-        try:
-            os.execv(sys.executable, [sys.executable] + sys.argv)
-        except OSError:
-            # Mac OS X versions prior to 10.6 do not support execv in
-            # a process that contains multiple threads.  Instead of
-            # re-executing in the current process, start a new one
-            # and cause the current process to exit.  This isn't
-            # ideal since the new process is detached from the parent
-            # terminal and thus cannot easily be killed with Ctrl-C,
-            # but it's better than not being able to autoreload at
-            # all.
-            # Unfortunately the errno returned in this case does not
-            # appear to be consistent, so we can't easily check for
-            # this error specifically.
-            os.spawnv(os.P_NOWAIT, sys.executable, [sys.executable] + sys.argv)
-            # At this point the IOLoop has been closed and finally
-            # blocks will experience errors if we allow the stack to
-            # unwind, so just exit uncleanly.
-            os._exit(0)

zerver/tornado/event_queue.py

@@ -37,6 +37,7 @@ import orjson
 import tornado.ioloop
 from django.conf import settings
 from django.utils.translation import gettext as _
+from tornado import autoreload
 
 from version import API_FEATURE_LEVEL, ZULIP_MERGE_BASE, ZULIP_VERSION
 from zerver.lib.exceptions import JsonableError
@@ -46,7 +47,6 @@ from zerver.lib.notification_data import UserMessageNotificationsData
 from zerver.lib.queue import queue_json_publish, retry_event
 from zerver.lib.utils import statsd
 from zerver.middleware import async_request_timer_restart
-from zerver.tornado.autoreload import add_reload_hook
 from zerver.tornado.descriptors import clear_descriptor_by_handler_id, set_descriptor_by_handler_id
 from zerver.tornado.exceptions import BadEventQueueIdError
 from zerver.tornado.handlers import (
@@ -622,7 +622,7 @@ def setup_event_queue(server: tornado.httpserver.HTTPServer, port: int) -> None:
             signal.SIGTERM,
             lambda signum, frame: ioloop.add_callback_from_signal(handle_sigterm, server),
         )
-        add_reload_hook(lambda: dump_event_queues(port))
+        autoreload.add_reload_hook(lambda: dump_event_queues(port))
 
     try:
         os.rename(persistent_queue_filename(port), persistent_queue_filename(port, last=True))

zproject/computed_settings.py

@@ -232,7 +232,6 @@ if not TORNADO_PORTS:
 TORNADO_PROCESSES = len(TORNADO_PORTS)
 
 RUNNING_INSIDE_TORNADO = False
-AUTORELOAD = DEBUG
 
 SILENCED_SYSTEM_CHECKS = [
     # auth.W004 checks that the UserProfile field named by USERNAME_FIELD has

zproject/test_extra_settings.py

@@ -113,9 +113,6 @@ else:
     WEBPACK_STATS_FILE = os.path.join(DEPLOY_ROOT, "var", "webpack-stats-test.json")
 WEBPACK_BUNDLES = "webpack-bundles/"
 
-# Don't auto-restart Tornado server during automated tests
-AUTORELOAD = False
-
 if not PUPPETEER_TESTS:
     # Use local memory cache for backend tests.
     CACHES["default"] = {