Fix reloader on OSX py38 and Windows (#1827)

* Fix watchdog reload worker repeatedly if there are multiple changed files

* Simplify autoreloader, don't need multiprocessing.Process. Now works on OSX py38.

* Allow autoreloader with multiple workers and run it earlier.

* This works OK on Windows too.

* I don't see how cwd could be different here.

* app.run and app.create_server argument fixup.

* Add test for auto_reload (coverage not working unfortunately).

* Reloader cleanup, don't use external kill commands and exit normally.

* Strip newlines on test output (Windows-compat).

* Report failures in test_auto_reload to avoid timeouts.

* Use different test server ports to avoid binding problems on Windows.

* Fix previous commit

* Listen on same port after reload.

* Show Goin' Fast banner on reloads.

* More robust testing, also -m sanic.

* Add a timeout to terminate process

* Try a workaround for tmpdir deletion on Windows.

* Join process also on error (context manager doesn't).

* Cleaner autoreloader termination on Windows.

* Remove unused code.

* Rename test.

* Longer timeout on test exit.

Co-authored-by: Hùng X. Lê <[email protected]>
Co-authored-by: L. Kärkkäinen <[email protected]>
Co-authored-by: Adam Hopkins <[email protected]>

sanic/app.py

@@ -1058,7 +1058,9 @@ def run(
         self,
         host: Optional[str] = None,
         port: Optional[int] = None,
+        *,
         debug: bool = False,
+        auto_reload: Optional[bool] = None,
         ssl: Union[dict, SSLContext, None] = None,
         sock: Optional[socket] = None,
         workers: int = 1,
@@ -1067,7 +1069,7 @@ def run(
         stop_event: Any = None,
         register_sys_signals: bool = True,
         access_log: Optional[bool] = None,
-        **kwargs: Any,
+        loop: None = None,
     ) -> None:
         """Run the HTTP Server and listen until keyboard interrupt or term
         signal. On termination, drain connections before closing.
@@ -1078,6 +1080,9 @@ def run(
         :type port: int
         :param debug: Enables debug output (slows server)
         :type debug: bool
+        :param auto_reload: Reload app whenever its source code is changed.
+                            Enabled by default in debug mode.
+        :type auto_relaod: bool
         :param ssl: SSLContext, or location of certificate and key
                     for SSL encryption of worker(s)
         :type ssl: SSLContext or dict
@@ -1099,21 +1104,17 @@ def run(
         :type access_log: bool
         :return: Nothing
         """
-        if "loop" in kwargs:
+        if loop is not None:
             raise TypeError(
                 "loop is not a valid argument. To use an existing loop, "
                 "change to create_server().\nSee more: "
                 "https://sanic.readthedocs.io/en/latest/sanic/deploying.html"
                 "#asynchronous-support"
             )
 
-        # Default auto_reload to false
-        auto_reload = False
-        # If debug is set, default it to true (unless on windows)
-        if debug and os.name == "posix":
-            auto_reload = True
-        # Allow for overriding either of the defaults
-        auto_reload = kwargs.get("auto_reload", auto_reload)
+        if auto_reload or auto_reload is None and debug:
+            if os.environ.get("SANIC_SERVER_RUNNING") != "true":
+                return reloader_helpers.watchdog(1.0)
 
         if sock is None:
             host, port = host or "127.0.0.1", port or 8000
@@ -1156,18 +1157,7 @@ def run(
                 )
                 workers = 1
             if workers == 1:
-                if auto_reload and os.name != "posix":
-                    # This condition must be removed after implementing
-                    # auto reloader for other operating systems.
-                    raise NotImplementedError
-
-                if (
-                    auto_reload
-                    and os.environ.get("SANIC_SERVER_RUNNING") != "true"
-                ):
-                    reloader_helpers.watchdog(2)
-                else:
-                    serve(**server_settings)
+                serve(**server_settings)
             else:
                 serve_multiple(server_settings, workers)
         except BaseException:
@@ -1189,6 +1179,7 @@ async def create_server(
         self,
         host: Optional[str] = None,
         port: Optional[int] = None,
+        *,
         debug: bool = False,
         ssl: Union[dict, SSLContext, None] = None,
         sock: Optional[socket] = None,
@@ -1413,7 +1404,7 @@ def _helper(
             server_settings["run_async"] = True
 
         # Serve
-        if host and port and os.environ.get("SANIC_SERVER_RUNNING") != "true":
+        if host and port:
             proto = "http"
             if ssl is not None:
                 proto = "https"

sanic/reloader_helpers.py

@@ -3,7 +3,6 @@
 import subprocess
 import sys
 
-from multiprocessing import Process
 from time import sleep
 
 
@@ -35,133 +34,70 @@ def _iter_module_files():
 
 def _get_args_for_reloading():
     """Returns the executable."""
-    rv = [sys.executable]
     main_module = sys.modules["__main__"]
     mod_spec = getattr(main_module, "__spec__", None)
+    if sys.argv[0] in ("", "-c"):
+        raise RuntimeError(
+            f"Autoreloader cannot work with argv[0]={sys.argv[0]!r}"
+        )
     if mod_spec:
         # Parent exe was launched as a module rather than a script
-        rv.extend(["-m", mod_spec.name])
-        if len(sys.argv) > 1:
-            rv.extend(sys.argv[1:])
-    else:
-        rv.extend(sys.argv)
-    return rv
+        return [sys.executable, "-m", mod_spec.name] + sys.argv[1:]
+    return [sys.executable] + sys.argv
 
 
 def restart_with_reloader():
     """Create a new process and a subprocess in it with the same arguments as
     this one.
     """
-    cwd = os.getcwd()
-    args = _get_args_for_reloading()
-    new_environ = os.environ.copy()
-    new_environ["SANIC_SERVER_RUNNING"] = "true"
-    cmd = " ".join(args)
-    worker_process = Process(
-        target=subprocess.call,
-        args=(cmd,),
-        kwargs={"cwd": cwd, "shell": True, "env": new_environ},
+    return subprocess.Popen(
+        _get_args_for_reloading(),
+        env={**os.environ, "SANIC_SERVER_RUNNING": "true"},
     )
-    worker_process.start()
-    return worker_process
-
 
-def kill_process_children_unix(pid):
-    """Find and kill child processes of a process (maximum two level).
-
-    :param pid: PID of parent process (process ID)
-    :return: Nothing
-    """
-    root_process_path = f"/proc/{pid}/task/{pid}/children"
-    if not os.path.isfile(root_process_path):
-        return
-    with open(root_process_path) as children_list_file:
-        children_list_pid = children_list_file.read().split()
-
-    for child_pid in children_list_pid:
-        children_proc_path = "/proc/%s/task/%s/children" % (
-            child_pid,
-            child_pid,
-        )
-        if not os.path.isfile(children_proc_path):
-            continue
-        with open(children_proc_path) as children_list_file_2:
-            children_list_pid_2 = children_list_file_2.read().split()
-        for _pid in children_list_pid_2:
-            try:
-                os.kill(int(_pid), signal.SIGTERM)
-            except ProcessLookupError:
-                continue
-        try:
-            os.kill(int(child_pid), signal.SIGTERM)
-        except ProcessLookupError:
-            continue
 
+def watchdog(sleep_interval):
+    """Watch project files, restart worker process if a change happened.
 
-def kill_process_children_osx(pid):
-    """Find and kill child processes of a process.
-
-    :param pid: PID of parent process (process ID)
+    :param sleep_interval: interval in second.
     :return: Nothing
     """
-    subprocess.run(["pkill", "-P", str(pid)])
 
+    def interrupt_self(*args):
+        raise KeyboardInterrupt
 
-def kill_process_children(pid):
-    """Find and kill child processes of a process.
-
-    :param pid: PID of parent process (process ID)
-    :return: Nothing
-    """
-    if sys.platform == "darwin":
-        kill_process_children_osx(pid)
-    elif sys.platform == "linux":
-        kill_process_children_unix(pid)
-    else:
-        pass  # should signal error here
+    mtimes = {}
+    signal.signal(signal.SIGTERM, interrupt_self)
+    if os.name == "nt":
+        signal.signal(signal.SIGBREAK, interrupt_self)
 
+    worker_process = restart_with_reloader()
 
-def kill_program_completly(proc):
-    """Kill worker and it's child processes and exit.
+    try:
+        while True:
+            need_reload = False
 
-    :param proc: worker process (process ID)
-    :return: Nothing
-    """
-    kill_process_children(proc.pid)
-    proc.terminate()
-    os._exit(0)
+            for filename in _iter_module_files():
+                try:
+                    mtime = os.stat(filename).st_mtime
+                except OSError:
+                    continue
 
+                old_time = mtimes.get(filename)
+                if old_time is None:
+                    mtimes[filename] = mtime
+                elif mtime > old_time:
+                    mtimes[filename] = mtime
+                    need_reload = True
 
-def watchdog(sleep_interval):
-    """Watch project files, restart worker process if a change happened.
-
-    :param sleep_interval: interval in second.
-    :return: Nothing
-    """
-    mtimes = {}
-    worker_process = restart_with_reloader()
-    signal.signal(
-        signal.SIGTERM, lambda *args: kill_program_completly(worker_process)
-    )
-    signal.signal(
-        signal.SIGINT, lambda *args: kill_program_completly(worker_process)
-    )
-    while True:
-        for filename in _iter_module_files():
-            try:
-                mtime = os.stat(filename).st_mtime
-            except OSError:
-                continue
-
-            old_time = mtimes.get(filename)
-            if old_time is None:
-                mtimes[filename] = mtime
-                continue
-            elif mtime > old_time:
-                kill_process_children(worker_process.pid)
+            if need_reload:
                 worker_process.terminate()
+                worker_process.wait()
                 worker_process = restart_with_reloader()
-                mtimes[filename] = mtime
-                break
 
-        sleep(sleep_interval)
+            sleep(sleep_interval)
+    except KeyboardInterrupt:
+        pass
+    finally:
+        worker_process.terminate()
+        worker_process.wait()

tests/test_reloader.py

@@ -0,0 +1,88 @@
+import os
+import secrets
+import sys
+
+from subprocess import PIPE, Popen
+from tempfile import TemporaryDirectory
+from textwrap import dedent
+from threading import Timer
+from time import sleep
+
+import pytest
+
+
+# We need to interrupt the autoreloader without killing it, so that the server gets terminated
+# https://stefan.sofa-rockers.org/2013/08/15/handling-sub-process-hierarchies-python-linux-os-x/
+
+try:
+    from signal import CTRL_BREAK_EVENT
+    from subprocess import CREATE_NEW_PROCESS_GROUP
+
+    flags = CREATE_NEW_PROCESS_GROUP
+except ImportError:
+    flags = 0
+
+def terminate(proc):
+    if flags:
+        proc.send_signal(CTRL_BREAK_EVENT)
+    else:
+        proc.terminate()
+
+def write_app(filename, **runargs):
+    text = secrets.token_urlsafe()
+    with open(filename, "w") as f:
+        f.write(dedent(f"""\
+            import os
+            from sanic import Sanic
+
+            app = Sanic(__name__)
+
+            @app.listener("after_server_start")
+            def complete(*args):
+                print("complete", os.getpid(), {text!r})
+
+            if __name__ == "__main__":
+                app.run(**{runargs!r})
+            """
+        ))
+    return text
+
+def scanner(proc):
+    for line in proc.stdout:
+        line = line.decode().strip()
+        print(">", line)
+        if line.startswith("complete"):
+            yield line
+
+
+argv = dict(
+    script=[sys.executable, "reloader.py"],
+    module=[sys.executable, "-m", "reloader"],
+    sanic=[sys.executable, "-m", "sanic", "--port", "42104", "--debug", "reloader.app"],
+)
+
+@pytest.mark.parametrize("runargs, mode", [
+    (dict(port=42102, auto_reload=True), "script"),
+    (dict(port=42103, debug=True), "module"),
+    (dict(), "sanic"),
+])
+async def test_reloader_live(runargs, mode):
+    with TemporaryDirectory() as tmpdir:
+        filename = os.path.join(tmpdir, "reloader.py")
+        text = write_app(filename, **runargs)
+        proc = Popen(argv[mode], cwd=tmpdir, stdout=PIPE, creationflags=flags)
+        try:
+            timeout = Timer(5, terminate, [proc])
+            timeout.start()
+            # Python apparently keeps using the old source sometimes if
+            # we don't sleep before rewrite (pycache timestamp problem?)
+            sleep(1)
+            line = scanner(proc)
+            assert text in next(line)
+            # Edit source code and try again
+            text = write_app(filename, **runargs)
+            assert text in next(line)
+        finally:
+            timeout.cancel()
+            terminate(proc)
+            proc.wait(timeout=3)