Repository: tornadoweb/tornado
Pull request #3302 (2023-07-27 01:20:04)

autoreload: Various updates
* Support running a directory instead of a module (#2855). In both the regular debug mode and CLI wrapper
* Use a real option parser for CLI wrapper
* Add `--until-success` flag (#2398)
* More tests!
* Remove some old compatibility code and update type annotations


Patch:
diff --git a/requirements.txt b/requirements.txt
index c01c92bf..a2d1ebc4 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -14,7 +14,7 @@ build==0.10.0
     # via pip-tools
 cachetools==5.3.1
     # via tox
-certifi==2023.7.22
+certifi==2022.12.7
     # via requests
 chardet==5.1.0
     # via tox
@@ -75,7 +75,7 @@ pycodestyle==2.10.0
     # via flake8
 pyflakes==3.0.1
     # via flake8
-pygments==2.15.0
+pygments==2.14.0
     # via sphinx
 pyproject-api==1.5.1
     # via tox
@@ -108,6 +108,13 @@ sphinxcontrib-qthelp==1.0.3
     # via sphinx
 sphinxcontrib-serializinghtml==1.1.5
     # via sphinx
+tomli==2.0.1
+    # via
+    #   black
+    #   build
+    #   mypy
+    #   pyproject-api
+    #   tox
 tox==4.6.0
     # via -r requirements.in
 types-pycurl==7.45.2.0
diff --git a/tornado/autoreload.py b/tornado/autoreload.py
index 0ac44966..a4522ecb 100644
--- a/tornado/autoreload.py
+++ b/tornado/autoreload.py
@@ -60,8 +60,7 @@ import sys
 # may become relative in spite of the future import.
 #
 # We address the former problem by reconstructing the original command
-# line (Python >= 3.4) or by setting the $PYTHONPATH environment
-# variable (Python < 3.4) before re-execution so the new process will
+# line before re-execution so the new process will
 # see the correct path.  We attempt to address the latter problem when
 # tornado.autoreload is run as __main__.
 
@@ -76,8 +75,9 @@ if __name__ == "__main__":
         del sys.path[0]
 
 import functools
+import importlib.abc
 import os
-import pkgutil  # type: ignore
+import pkgutil
 import sys
 import traceback
 import types
@@ -87,18 +87,13 @@ import weakref
 from tornado import ioloop
 from tornado.log import gen_log
 from tornado import process
-from tornado.util import exec_in
 
 try:
     import signal
 except ImportError:
     signal = None  # type: ignore
 
-import typing
-from typing import Callable, Dict
-
-if typing.TYPE_CHECKING:
-    from typing import List, Optional, Union  # noqa: F401
+from typing import Callable, Dict, Optional, List, Union
 
 # os.execv is broken on Windows and can't properly parse command line
 # arguments and executable name if they contain whitespaces. subprocess
@@ -108,9 +103,11 @@ _has_execv = sys.platform != "win32"
 _watched_files = set()
 _reload_hooks = []
 _reload_attempted = False
-_io_loops = weakref.WeakKeyDictionary()  # type: ignore
+_io_loops: "weakref.WeakKeyDictionary[ioloop.IOLoop, bool]" = (
+    weakref.WeakKeyDictionary()
+)
 _autoreload_is_main = False
-_original_argv = None  # type: Optional[List[str]]
+_original_argv: Optional[List[str]] = None
 _original_spec = None
 
 
@@ -126,7 +123,7 @@ def start(check_time: int = 500) -> None:
     _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 = {}  # type: Dict[str, float]
+    modify_times: Dict[str, float] = {}
     callback = functools.partial(_reload_on_update, modify_times)
     scheduler = ioloop.PeriodicCallback(callback, check_time)
     scheduler.start()
@@ -214,10 +211,7 @@ def _reload() -> None:
     # sys.path fixes: see comments at top of file.  If __main__.__spec__
     # exists, we were invoked with -m and the effective path is about to
     # change on re-exec.  Reconstruct the original command line to
-    # ensure that the new process sees the same path we did.  If
-    # __spec__ is not available (Python < 3.4), check instead if
-    # sys.path[0] is an empty string and add the current directory to
-    # $PYTHONPATH.
+    # ensure that the new process sees the same path we did.
     if _autoreload_is_main:
         assert _original_argv is not None
         spec = _original_spec
@@ -225,43 +219,25 @@ def _reload() -> None:
     else:
         spec = getattr(sys.modules["__main__"], "__spec__", None)
         argv = sys.argv
-    if spec:
+    if spec and spec.name != "__main__":
+        # __spec__ is set in two cases: when running a module, and when running a directory. (when
+        # running a file, there is no spec). In the former case, we must pass -m to maintain the
+        # module-style behavior (setting sys.path), even though python stripped -m from its argv at
+        # startup. If sys.path is exactly __main__, we're running a directory and should fall
+        # through to the non-module behavior.
+        #
+        # Some of this, including the use of exactly __main__ as a spec for directory mode,
+        # is documented at https://docs.python.org/3/library/runpy.html#runpy.run_path
         argv = ["-m", spec.name] + argv[1:]
-    else:
-        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] + argv)
         os._exit(0)
     else:
-        try:
-            os.execv(sys.executable, [sys.executable] + 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] + argv  # type: ignore
-            )
-            # 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)
-
-
-_USAGE = """\
-Usage:
+        os.execv(sys.executable, [sys.executable] + argv)
+
+
+_USAGE = """
   python -m tornado.autoreload -m module.to.run [args...]
   python -m tornado.autoreload path/to/script.py [args...]
 """
@@ -283,6 +259,12 @@ def main() -> None:
     # Remember that we were launched with autoreload as main.
     # The main module can be tricky; set the variables both in our globals
     # (which may be __main__) and the real importable version.
+    #
+    # We use optparse instead of the newer argparse because we want to
+    # mimic the python command-line interface which requires stopping
+    # parsing at the first positional argument. optparse supports
+    # this but as far as I can tell argparse does not.
+    import optparse
     import tornado.autoreload
 
     global _autoreload_is_main
@@ -292,38 +274,43 @@ def main() -> None:
     tornado.autoreload._original_argv = _original_argv = original_argv
     original_spec = getattr(sys.modules["__main__"], "__spec__", None)
     tornado.autoreload._original_spec = _original_spec = original_spec
-    sys.argv = sys.argv[:]
-    if len(sys.argv) >= 3 and sys.argv[1] == "-m":
-        mode = "module"
-        module = sys.argv[2]
-        del sys.argv[1:3]
-    elif len(sys.argv) >= 2:
-        mode = "script"
-        script = sys.argv[1]
-        sys.argv = sys.argv[1:]
+
+    parser = optparse.OptionParser(
+        prog="python -m tornado.autoreload",
+        usage=_USAGE,
+        epilog="Either -m or a path must be specified, but not both",
+    )
+    parser.disable_interspersed_args()
+    parser.add_option("-m", dest="module", metavar="module", help="module to run")
+    parser.add_option(
+        "--until-success",
+        action="store_true",
+        help="stop reloading after the program exist successfully (status code 0)",
+    )
+    opts, rest = parser.parse_args()
+    if opts.module is None:
+        if not rest:
+            print("Either -m or a path must be specified", file=sys.stderr)
+            sys.exit(1)
+        path = rest[0]
+        sys.argv = rest[:]
     else:
-        print(_USAGE, file=sys.stderr)
-        sys.exit(1)
+        path = None
+        sys.argv = [sys.argv[0]] + rest
 
+    # SystemExit.code is typed funny: https://github.com/python/typeshed/issues/8513
+    # All we care about is truthiness
+    exit_status: Union[int, str, None] = 1
     try:
-        if mode == "module":
-            import runpy
-
-            runpy.run_module(module, run_name="__main__", alter_sys=True)
-        elif mode == "script":
-            with open(script) as f:
-                # Execute the script in our namespace instead of creating
-                # a new one so that something that tries to import __main__
-                # (e.g. the unittest module) will see names defined in the
-                # script instead of just those defined in this module.
-                global __file__
-                __file__ = script
-                # If __package__ is defined, imports may be incorrectly
-                # interpreted as relative to this module.
-                global __package__
-                del __package__
-                exec_in(f.read(), globals(), globals())
+        import runpy
+
+        if opts.module is not None:
+            runpy.run_module(opts.module, run_name="__main__", alter_sys=True)
+        else:
+            assert path is not None
+            runpy.run_path(path, run_name="__main__")
     except SystemExit as e:
+        exit_status = e.code
         gen_log.info("Script exited with status %s", e.code)
     except Exception as e:
         gen_log.warning("Script exited with uncaught exception", exc_info=True)
@@ -331,7 +318,7 @@ def main() -> None:
         # never made it into sys.modules and so we won't know to watch it.
         # Just to make sure we've covered everything, walk the stack trace
         # from the exception and watch every file.
-        for (filename, lineno, name, line) in traceback.extract_tb(sys.exc_info()[2]):
+        for filename, lineno, name, line in traceback.extract_tb(sys.exc_info()[2]):
             watch(filename)
         if isinstance(e, SyntaxError):
             # SyntaxErrors are special:  their innermost stack frame is fake
@@ -340,17 +327,22 @@ def main() -> None:
             if e.filename is not None:
                 watch(e.filename)
     else:
+        exit_status = 0
         gen_log.info("Script exited normally")
     # restore sys.argv so subsequent executions will include autoreload
     sys.argv = original_argv
 
-    if mode == "module":
+    if opts.module is not None:
+        assert opts.module is not None
         # runpy did a fake import of the module as __main__, but now it's
         # no longer in sys.modules.  Figure out where it is and watch it.
-        loader = pkgutil.get_loader(module)
-        if loader is not None:
+        loader = pkgutil.get_loader(opts.module)
+        if loader is not None and isinstance(loader, importlib.abc.FileLoader):
+            # TODO: fix when we update typeshed
             watch(loader.get_filename())  # type: ignore
 
+    if opts.until_success and not exit_status:
+        return
     wait()
 
 


Test patch:
diff --git a/tornado/test/autoreload_test.py b/tornado/test/autoreload_test.py
index fff60281..5675faa2 100644
--- a/tornado/test/autoreload_test.py
+++ b/tornado/test/autoreload_test.py
@@ -4,14 +4,44 @@ import subprocess
 from subprocess import Popen
 import sys
 from tempfile import mkdtemp
+import textwrap
 import time
 import unittest
 
 
 class AutoreloadTest(unittest.TestCase):
     def setUp(self):
+        # When these tests fail the output sometimes exceeds the default maxDiff.
+        self.maxDiff = 1024
+
         self.path = mkdtemp()
 
+        # Most test apps run themselves twice via autoreload. The first time it manually triggers
+        # a reload (could also do this by touching a file but this is faster since filesystem
+        # timestamps are not necessarily high resolution). The second time it exits directly
+        # so that the autoreload wrapper (if it is used) doesn't catch it.
+        #
+        # The last line of each such test's "main" program should be
+        #     exec(open("run_twice_magic.py").read())
+        self.write_files(
+            {
+                "run_twice_magic.py": """
+                    import os
+                    import sys
+
+                    import tornado.autoreload
+
+                    sys.stdout.flush()
+
+                    if "TESTAPP_STARTED" not in os.environ:
+                        os.environ["TESTAPP_STARTED"] = "1"
+                        tornado.autoreload._reload()
+                    else:
+                        os._exit(0)
+                """
+            }
+        )
+
     def tearDown(self):
         try:
             shutil.rmtree(self.path)
@@ -24,33 +54,23 @@ class AutoreloadTest(unittest.TestCase):
             time.sleep(1)
             shutil.rmtree(self.path)
 
-    def test_reload_module(self):
-        main = """\
-import os
-import sys
+    def write_files(self, tree, base_path=None):
+        """Write a directory tree to self.path.
 
-from tornado import autoreload
-
-# This import will fail if path is not set up correctly
-import testapp
-
-print('Starting')
-if 'TESTAPP_STARTED' not in os.environ:
-    os.environ['TESTAPP_STARTED'] = '1'
-    sys.stdout.flush()
-    autoreload._reload()
-"""
-
-        # Create temporary test application
-        os.mkdir(os.path.join(self.path, "testapp"))
-        open(
-            os.path.join(self.path, "testapp/__init__.py"), "w", encoding="utf-8"
-        ).close()
-        with open(
-            os.path.join(self.path, "testapp/__main__.py"), "w", encoding="utf-8"
-        ) as f:
-            f.write(main)
+        tree is a dictionary mapping file names to contents, or
+        sub-dictionaries representing subdirectories.
+        """
+        if base_path is None:
+            base_path = self.path
+        for name, contents in tree.items():
+            if isinstance(contents, dict):
+                os.mkdir(os.path.join(base_path, name))
+                self.write_files(contents, os.path.join(base_path, name))
+            else:
+                with open(os.path.join(base_path, name), "w", encoding="utf-8") as f:
+                    f.write(textwrap.dedent(contents))
 
+    def run_subprocess(self, args):
         # Make sure the tornado module under test is available to the test
         # application
         pythonpath = os.getcwd()
@@ -58,22 +78,114 @@ if 'TESTAPP_STARTED' not in os.environ:
             pythonpath += os.pathsep + os.environ["PYTHONPATH"]
 
         p = Popen(
-            [sys.executable, "-m", "testapp"],
+            args,
             stdout=subprocess.PIPE,
-            cwd=self.path,
             env=dict(os.environ, PYTHONPATH=pythonpath),
+            cwd=self.path,
             universal_newlines=True,
             encoding="utf-8",
         )
+
+        # This timeout needs to be fairly generous for pypy due to jit
+        # warmup costs.
+        for i in range(40):
+            if p.poll() is not None:
+                break
+            time.sleep(0.1)
+        else:
+            p.kill()
+            raise Exception("subprocess failed to terminate")
+
         out = p.communicate()[0]
-        self.assertEqual(out, "Starting\nStarting\n")
+        self.assertEqual(p.returncode, 0)
+        return out
+
+    def test_reload(self):
+        main = """\
+import sys
+
+# In module mode, the path is set to the parent directory and we can import testapp.
+try:
+    import testapp
+except ImportError:
+    print("import testapp failed")
+else:
+    print("import testapp succeeded")
+
+spec = getattr(sys.modules[__name__], '__spec__', None)
+print(f"Starting {__name__=}, __spec__.name={getattr(spec, 'name', None)}")
+exec(open("run_twice_magic.py").read())
+"""
+
+        # Create temporary test application
+        self.write_files(
+            {
+                "testapp": {
+                    "__init__.py": "",
+                    "__main__.py": main,
+                },
+            }
+        )
+
+        # The autoreload wrapper should support all the same modes as the python interpreter.
+        # The wrapper itself should have no effect on this test so we try all modes with and
+        # without it.
+        for wrapper in [False, True]:
+            with self.subTest(wrapper=wrapper):
+                with self.subTest(mode="module"):
+                    if wrapper:
+                        base_args = [sys.executable, "-m", "tornado.autoreload"]
+                    else:
+                        base_args = [sys.executable]
+                    # In module mode, the path is set to the parent directory and we can import
+                    # testapp. Also, the __spec__.name is set to the fully qualified module name.
+                    out = self.run_subprocess(base_args + ["-m", "testapp"])
+                    self.assertEqual(
+                        out,
+                        (
+                            "import testapp succeeded\n"
+                            + "Starting __name__='__main__', __spec__.name=testapp.__main__\n"
+                        )
+                        * 2,
+                    )
+
+                with self.subTest(mode="file"):
+                    out = self.run_subprocess(base_args + ["testapp/__main__.py"])
+                    # In file mode, we do not expect the path to be set so we can import testapp,
+                    # but when the wrapper is used the -m argument to the python interpreter
+                    # does this for us.
+                    expect_import = (
+                        "import testapp succeeded"
+                        if wrapper
+                        else "import testapp failed"
+                    )
+                    # In file mode there is no qualified module spec.
+                    self.assertEqual(
+                        out,
+                        f"{expect_import}\nStarting __name__='__main__', __spec__.name=None\n"
+                        * 2,
+                    )
+
+                with self.subTest(mode="directory"):
+                    # Running as a directory finds __main__.py like a module. It does not manipulate
+                    # sys.path but it does set a spec with a name of exactly __main__.
+                    out = self.run_subprocess(base_args + ["testapp"])
+                    expect_import = (
+                        "import testapp succeeded"
+                        if wrapper
+                        else "import testapp failed"
+                    )
+                    self.assertEqual(
+                        out,
+                        f"{expect_import}\nStarting __name__='__main__', __spec__.name=__main__\n"
+                        * 2,
+                    )
 
     def test_reload_wrapper_preservation(self):
         # This test verifies that when `python -m tornado.autoreload`
         # is used on an application that also has an internal
         # autoreload, the reload wrapper is preserved on restart.
         main = """\
-import os
 import sys
 
 # This import will fail if path is not set up correctly
@@ -82,52 +194,71 @@ import testapp
 if 'tornado.autoreload' not in sys.modules:
     raise Exception('started without autoreload wrapper')
 
-import tornado.autoreload
-
 print('Starting')
-sys.stdout.flush()
-if 'TESTAPP_STARTED' not in os.environ:
-    os.environ['TESTAPP_STARTED'] = '1'
-    # Simulate an internal autoreload (one not caused
-    # by the wrapper).
-    tornado.autoreload._reload()
-else:
-    # Exit directly so autoreload doesn't catch it.
-    os._exit(0)
+exec(open("run_twice_magic.py").read())
 """
 
+        self.write_files(
+            {
+                "testapp": {
+                    "__init__.py": "",
+                    "__main__.py": main,
+                },
+            }
+        )
+
+        out = self.run_subprocess(
+            [sys.executable, "-m", "tornado.autoreload", "-m", "testapp"]
+        )
+        self.assertEqual(out, "Starting\n" * 2)
+
+    def test_reload_wrapper_args(self):
+        main = """\
+import os
+import sys
+
+print(os.path.basename(sys.argv[0]))
+print(f'argv={sys.argv[1:]}')
+exec(open("run_twice_magic.py").read())
+"""
         # Create temporary test application
-        os.mkdir(os.path.join(self.path, "testapp"))
-        init_file = os.path.join(self.path, "testapp", "__init__.py")
-        open(init_file, "w", encoding="utf-8").close()
-        main_file = os.path.join(self.path, "testapp", "__main__.py")
-        with open(main_file, "w", encoding="utf-8") as f:
-            f.write(main)
+        self.write_files({"main.py": main})
 
         # Make sure the tornado module under test is available to the test
         # application
-        pythonpath = os.getcwd()
-        if "PYTHONPATH" in os.environ:
-            pythonpath += os.pathsep + os.environ["PYTHONPATH"]
-
-        autoreload_proc = Popen(
-            [sys.executable, "-m", "tornado.autoreload", "-m", "testapp"],
-            stdout=subprocess.PIPE,
-            cwd=self.path,
-            env=dict(os.environ, PYTHONPATH=pythonpath),
-            universal_newlines=True,
-            encoding="utf-8",
+        out = self.run_subprocess(
+            [
+                sys.executable,
+                "-m",
+                "tornado.autoreload",
+                "main.py",
+                "arg1",
+                "--arg2",
+                "-m",
+                "arg3",
+            ],
         )
 
-        # This timeout needs to be fairly generous for pypy due to jit
-        # warmup costs.
-        for i in range(40):
-            if autoreload_proc.poll() is not None:
-                break
-            time.sleep(0.1)
-        else:
-            autoreload_proc.kill()
-            raise Exception("subprocess failed to terminate")
+        self.assertEqual(out, "main.py\nargv=['arg1', '--arg2', '-m', 'arg3']\n" * 2)
 
-        out = autoreload_proc.communicate()[0]
-        self.assertEqual(out, "Starting\n" * 2)
+    def test_reload_wrapper_until_success(self):
+        main = """\
+import os
+import sys
+
+if "TESTAPP_STARTED" in os.environ:
+    print("exiting cleanly")
+    sys.exit(0)
+else:
+    print("reloading")
+    exec(open("run_twice_magic.py").read())
+"""
+
+        # Create temporary test application
+        self.write_files({"main.py": main})
+
+        out = self.run_subprocess(
+            [sys.executable, "-m", "tornado.autoreload", "--until-success", "main.py"]
+        )
+
+        self.assertEqual(out, "reloading\nexiting cleanly\n")