Repository: tornadoweb/tornado

Issue:
autoreload: Fix argv preservation
`autoreload` currently has a wrapper mode (e.g. `python -m tornado.autoreload -m tornado.test`) for scripts, and an in-process mode (enabled by `Application(..., debug=True)`). It's useful to combine these, since the wrapper can catch syntax errors that cause the process to abort before entering its IOLoop. However, this doesn't work as well as it should, because the `main` wrapper only restores `sys.argv` if the process exits, meaning the `-m tornado.autoreload` flags are lost if the inner autoreload fires. The original argv needs to be stored in a global when `autoreload` is `__main__`, so that it can be used in `_reload()`.

Discussion:

Patch:
diff --git a/tornado/autoreload.py b/tornado/autoreload.py
index 2f911270..7d69474a 100644
--- a/tornado/autoreload.py
+++ b/tornado/autoreload.py
@@ -107,6 +107,9 @@ _watched_files = set()
 _reload_hooks = []
 _reload_attempted = False
 _io_loops = weakref.WeakKeyDictionary()  # type: ignore
+_autoreload_is_main = False
+_original_argv = None
+_original_spec = None
 
 
 def start(check_time=500):
@@ -214,11 +217,15 @@ def _reload():
     # __spec__ is not available (Python < 3.4), check instead if
     # sys.path[0] is an empty string and add the current directory to
     # $PYTHONPATH.
-    spec = getattr(sys.modules['__main__'], '__spec__', None)
-    if spec:
-        argv = ['-m', spec.name] + sys.argv[1:]
+    if _autoreload_is_main:
+        spec = _original_spec
+        argv = _original_argv
     else:
+        spec = getattr(sys.modules['__main__'], '__spec__', None)
         argv = sys.argv
+    if spec:
+        argv = ['-m', spec.name] + argv[1:]
+    else:
         path_prefix = '.' + os.pathsep
         if (sys.path[0] == '' and
                 not os.environ.get("PYTHONPATH", "").startswith(path_prefix)):
@@ -226,7 +233,7 @@ def _reload():
                                         os.environ.get("PYTHONPATH", ""))
     if not _has_execv:
         subprocess.Popen([sys.executable] + argv)
-        sys.exit(0)
+        os._exit(0)
     else:
         try:
             os.execv(sys.executable, [sys.executable] + argv)
@@ -269,7 +276,17 @@ def main():
     can catch import-time problems like syntax errors that would otherwise
     prevent the script from reaching its call to `wait`.
     """
+    # 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.
+    import tornado.autoreload
+    global _autoreload_is_main
+    global _original_argv, _original_spec
+    tornado.autoreload._autoreload_is_main = _autoreload_is_main = True
     original_argv = sys.argv
+    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"
diff --git a/tornado/iostream.py b/tornado/iostream.py
index 89e1e234..63110a1a 100644
--- a/tornado/iostream.py
+++ b/tornado/iostream.py
@@ -1410,13 +1410,7 @@ class IOStream(BaseIOStream):
         return future
 
     def _handle_connect(self):
-        try:
-            err = self.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR)
-        except socket.error as e:
-            # Hurd doesn't allow SO_ERROR for loopback sockets because all
-            # errors for such sockets are reported synchronously.
-            if errno_from_exception(e) == errno.ENOPROTOOPT:
-                err = 0
+        err = self.socket.getsockopt(socket.SOL_SOCKET, socket.SO_ERROR)
         if err != 0:
             self.error = socket.error(err, os.strerror(err))
             # IOLoop implementations may vary: some of them return
diff --git a/tornado/netutil.py b/tornado/netutil.py
index e63683ad..08c9d886 100644
--- a/tornado/netutil.py
+++ b/tornado/netutil.py
@@ -138,12 +138,7 @@ def bind_sockets(port, address=None, family=socket.AF_UNSPEC,
             raise
         set_close_exec(sock.fileno())
         if os.name != 'nt':
-            try:
-                sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
-            except socket.error as e:
-                if errno_from_exception(e) != errno.ENOPROTOOPT:
-                    # Hurd doesn't support SO_REUSEADDR.
-                    raise
+            sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
         if reuse_port:
             sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1)
         if af == socket.AF_INET6:
@@ -185,12 +180,7 @@ if hasattr(socket, 'AF_UNIX'):
         """
         sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
         set_close_exec(sock.fileno())
-        try:
-            sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
-        except socket.error as e:
-            if errno_from_exception(e) != errno.ENOPROTOOPT:
-                # Hurd doesn't support SO_REUSEADDR
-                raise
+        sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
         sock.setblocking(0)
         try:
             st = os.stat(file)
diff --git a/tornado/web.py b/tornado/web.py
index 6760b0b9..f970bd13 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -749,18 +749,7 @@ class RequestHandler(object):
         self._write_buffer.append(chunk)
 
     def render(self, template_name, **kwargs):
-        """Renders the template with the given arguments as the response.
-
-        ``render()`` calls ``finish()``, so no other output methods can be called
-        after it.
-
-        Returns a `.Future` with the same semantics as the one returned by `finish`.
-        Awaiting this `.Future` is optional.
-
-        .. versionchanged:: 5.1
-
-           Now returns a `.Future` instead of ``None``.
-        """
+        """Renders the template with the given arguments as the response."""
         if self._finished:
             raise RuntimeError("Cannot render() after finish()")
         html = self.render_string(template_name, **kwargs)
@@ -821,7 +810,7 @@ class RequestHandler(object):
         if html_bodies:
             hloc = html.index(b'</body>')
             html = html[:hloc] + b''.join(html_bodies) + b'\n' + html[hloc:]
-        return self.finish(html)
+        self.finish(html)
 
     def render_linked_js(self, js_files):
         """Default method used to render the final js links for the
@@ -1004,20 +993,7 @@ class RequestHandler(object):
                 return future
 
     def finish(self, chunk=None):
-        """Finishes this response, ending the HTTP request.
-
-        Passing a ``chunk`` to ``finish()`` is equivalent to passing that
-        chunk to ``write()`` and then calling ``finish()`` with no arguments.
-
-        Returns a `.Future` which may optionally be awaited to track the sending
-        of the response to the client. This `.Future` resolves when all the response
-        data has been sent, and raises an error if the connection is closed before all
-        data can be sent.
-
-        .. versionchanged:: 5.1
-
-           Now returns a `.Future` instead of ``None``.
-        """
+        """Finishes this response, ending the HTTP request."""
         if self._finished:
             raise RuntimeError("finish() called twice")
 
@@ -1049,13 +1025,12 @@ class RequestHandler(object):
             # are keepalive connections)
             self.request.connection.set_close_callback(None)
 
-        future = self.flush(include_footers=True)
+        self.flush(include_footers=True)
         self.request.connection.finish()
         self._log()
         self._finished = True
         self.on_finish()
         self._break_cycles()
-        return future
 
     def detach(self):
         """Take control of the underlying stream.


Test patch:
diff --git a/tornado/test/autoreload_test.py b/tornado/test/autoreload_test.py
index 6a9729db..1ea53167 100644
--- a/tornado/test/autoreload_test.py
+++ b/tornado/test/autoreload_test.py
@@ -1,14 +1,19 @@
 from __future__ import absolute_import, division, print_function
 import os
+import shutil
 import subprocess
 from subprocess import Popen
 import sys
 from tempfile import mkdtemp
+import time
 
 from tornado.test.util import unittest
 
 
-MAIN = """\
+class AutoreloadTest(unittest.TestCase):
+
+    def test_reload_module(self):
+        main = """\
 import os
 import sys
 
@@ -24,15 +29,13 @@ if 'TESTAPP_STARTED' not in os.environ:
     autoreload._reload()
 """
 
-
-class AutoreloadTest(unittest.TestCase):
-    def test_reload_module(self):
         # Create temporary test application
         path = mkdtemp()
+        self.addCleanup(shutil.rmtree, path)
         os.mkdir(os.path.join(path, 'testapp'))
         open(os.path.join(path, 'testapp/__init__.py'), 'w').close()
         with open(os.path.join(path, 'testapp/__main__.py'), 'w') as f:
-            f.write(MAIN)
+            f.write(main)
 
         # Make sure the tornado module under test is available to the test
         # application
@@ -46,3 +49,64 @@ class AutoreloadTest(unittest.TestCase):
             universal_newlines=True)
         out = p.communicate()[0]
         self.assertEqual(out, 'Starting\nStarting\n')
+
+    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
+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)
+"""
+
+        # Create temporary test application
+        path = mkdtemp()
+        os.mkdir(os.path.join(path, 'testapp'))
+        self.addCleanup(shutil.rmtree, path)
+        init_file = os.path.join(path, 'testapp', '__init__.py')
+        open(init_file, 'w').close()
+        main_file = os.path.join(path, 'testapp', '__main__.py')
+        with open(main_file, 'w') as f:
+            f.write(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=path,
+            env=dict(os.environ, PYTHONPATH=pythonpath),
+            universal_newlines=True)
+
+        for i in range(20):
+            if autoreload_proc.poll() is not None:
+                break
+            time.sleep(0.1)
+        else:
+            autoreload_proc.kill()
+            raise Exception("subprocess failed to terminate")
+
+        out = autoreload_proc.communicate()[0]
+        self.assertEqual(out, 'Starting\n' * 2)
diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py
index b77311df..45072aac 100644
--- a/tornado/test/web_test.py
+++ b/tornado/test/web_test.py
@@ -191,40 +191,6 @@ class SecureCookieV2Test(unittest.TestCase):
         self.assertEqual(new_handler.get_secure_cookie('foo'), None)
 
 
-class FinalReturnTest(WebTestCase):
-    def get_handlers(self):
-        test = self
-
-        class FinishHandler(RequestHandler):
-            @gen.coroutine
-            def get(self):
-                test.final_return = self.finish()
-
-        class RenderHandler(RequestHandler):
-            def create_template_loader(self, path):
-                return DictLoader({'foo.html': 'hi'})
-
-            @gen.coroutine
-            def get(self):
-                test.final_return = self.render('foo.html')
-
-        return [("/finish", FinishHandler),
-                ("/render", RenderHandler)]
-
-    def get_app_kwargs(self):
-        return dict(template_path='FinalReturnTest')
-
-    def test_finish_method_return_future(self):
-        response = self.fetch(self.get_url('/finish'))
-        self.assertEqual(response.code, 200)
-        self.assertIsInstance(self.final_return, Future)
-
-    def test_render_method_return_future(self):
-        response = self.fetch(self.get_url('/render'))
-        self.assertEqual(response.code, 200)
-        self.assertIsInstance(self.final_return, Future)
-
-
 class CookieTest(WebTestCase):
     def get_handlers(self):
         class SetCookieHandler(RequestHandler):