Merge pull request #597 from untitaker/reloader_fixes

More bugfixes for reloader

tests/conftest.py

@@ -20,10 +20,10 @@
 except ImportError:  # pragma: no cover
     from urllib.request import urlopen
 
-
 import pytest
-from werkzeug import serving
 
+from werkzeug import serving
+from werkzeug.utils import cached_property
 from werkzeug._compat import to_bytes
 
 
@@ -39,6 +39,15 @@ def subprocess(xprocess):
         return xprocess
 
 
+def _patch_reloader_loop():
+    def f(x):
+        print('reloader loop finished')
+        return time.sleep(x)
+
+    import werkzeug._reloader
+    werkzeug._reloader.ReloaderLoop._sleep = staticmethod(f)
+
+
 def _get_pid_middleware(f):
     def inner(environ, start_response):
         if environ['PATH_INFO'] == '/_getpid':
@@ -49,9 +58,8 @@ def inner(environ, start_response):
 
 
 def _dev_server():
-    appfile = sys.argv[1]
-
-    sys.path.insert(0, os.path.dirname(appfile))
+    _patch_reloader_loop()
+    sys.path.insert(0, sys.argv[1])
     import testsuite_app
     app = _get_pid_middleware(testsuite_app.app)
     serving.run_simple(hostname='localhost', application=app,
@@ -62,9 +70,50 @@ def _dev_server():
 
 
 class _ServerInfo(object):
+    xprocess = None
     addr = None
     url = None
     port = None
+    last_pid = None
+
+    def __init__(self, xprocess, addr, url, port):
+        self.xprocess = xprocess
+        self.addr = addr
+        self.url = url
+        self.port = port
+
+    @cached_property
+    def logfile(self):
+        return self.xprocess.getinfo('dev_server').logpath.open()
+
+    def request_pid(self):
+        for i in range(20):
+            time.sleep(0.1 * i)
+            try:
+                self.last_pid = int(urlopen(self.url + '/_getpid').read())
+                return self.last_pid
+            except Exception as e:  # urllib also raises socketerrors
+                print(self.url)
+                print(e)
+        return False
+
+    def wait_for_reloader(self):
+        old_pid = self.last_pid
+        for i in range(20):
+            time.sleep(0.1 * i)
+            new_pid = self.request_pid()
+            if not new_pid:
+                raise RuntimeError('Server is down.')
+            if self.request_pid() != old_pid:
+                return
+        raise RuntimeError('Server did not reload.')
+
+    def wait_for_reloader_loop(self):
+        for i in range(20):
+            time.sleep(0.1 * i)
+            line = self.logfile.readline()
+            if 'reloader loop finished' in line:
+                return
 
 
 @pytest.fixture
@@ -76,7 +125,8 @@ def dev_server(tmpdir, subprocess, request, monkeypatch):
         whose values will be passed to ``run_simple``.
     '''
     def run_dev_server(application):
-        appfile = tmpdir.join('testsuite_app.py')
+        app_pkg = tmpdir.mkdir('testsuite_app')
+        appfile = app_pkg.join('__init__.py')
         appfile.write('\n\n'.join((
             'kwargs = dict(port=5001)',
             textwrap.dedent(application)
@@ -92,34 +142,27 @@ def run_dev_server(application):
         else:
             url_base = 'http://localhost:{0}'.format(port)
 
-        def request_pid():
-            for i in range(10):
-                try:
-                    return int(urlopen(url_base + '/_getpid').read())
-                except Exception as e:  # urllib also raises socketerrors
-                    print(url_base)
-                    print(e)
-                    time.sleep(0.1 * i)
-            return False
+        info = _ServerInfo(
+            subprocess,
+            'localhost:{0}'.format(port),
+            url_base,
+            port
+        )
 
         def preparefunc(cwd):
-            args = [sys.executable, __file__, str(appfile)]
-            return request_pid, args
+            args = [sys.executable, __file__, str(tmpdir)]
+            return info.request_pid, args
 
         subprocess.ensure('dev_server', preparefunc, restart=True)
 
         def teardown():
             # Killing the process group that runs the server, not just the
             # parent process attached. xprocess is confused about Werkzeug's
             # reloader and won't help here.
-            pid = request_pid()
+            pid = info.last_pid
             os.killpg(os.getpgid(pid), signal.SIGTERM)
         request.addfinalizer(teardown)
 
-        rv = _ServerInfo()
-        rv.addr = 'localhost:{0}'.format(port)
-        rv.url = url_base
-        rv.port = port
-        return rv
+        return info
 
     return run_dev_server

tests/test_serving.py

@@ -122,7 +122,15 @@ def test_make_ssl_devcert(tmpdir):
     assert os.path.isfile(private_key)
 
 
-def test_reloader_broken_imports(tmpdir, dev_server):
+@pytest.mark.parametrize('reloader_type', ['watchdog', 'stat'])
+def test_reloader_broken_imports(tmpdir, dev_server, reloader_type):
+    # We explicitly assert that the server reloads on change, even though in
+    # this case the import could've just been retried. This is to assert
+    # correct behavior for apps that catch and cache import errors.
+
+    real_app = tmpdir.join('real_app.py')
+    real_app.write("lol syntax error")
+
     server = dev_server('''
     trials = []
     def app(environ, start_response):
@@ -133,12 +141,9 @@ def app(environ, start_response):
 
     kwargs['use_reloader'] = True
     kwargs['reloader_interval'] = 0.1
-    ''')
-
-    assert any('app.py' in str(x) for x in tmpdir.listdir())
-    real_app = tmpdir.join('real_app.py')
-    real_app.write("lol syntax error")
-    time.sleep(2)  # wait for stat reloader to pick up new file
+    kwargs['reloader_type'] = %s
+    ''' % repr(reloader_type))
+    server.wait_for_reloader_loop()
 
     connection = httplib.HTTPConnection(server.addr)
     connection.request('GET', '/')
@@ -150,7 +155,46 @@ def real_app(environ, start_response):
         start_response('200 OK', [('Content-Type', 'text/html')])
         return [b'hello']
     '''))
-    time.sleep(2)  # wait for stat reloader to pick up changes
+    server.wait_for_reloader()
+
+    connection.request('GET', '/')
+    response = connection.getresponse()
+    assert response.status == 200
+    assert response.read() == b'hello'
+
+
+@pytest.mark.parametrize('reloader_type', ['watchdog', 'stat'])
+def test_reloader_nested_broken_imports(tmpdir, dev_server, reloader_type):
+    real_app = tmpdir.mkdir('real_app')
+    real_app.join('__init__.py').write('from real_app.sub import real_app')
+    sub = real_app.mkdir('sub').join('__init__.py')
+    sub.write("lol syntax error")
+
+    server = dev_server('''
+    trials = []
+    def app(environ, start_response):
+        assert not trials, 'should have reloaded'
+        trials.append(1)
+        import real_app
+        return real_app.real_app(environ, start_response)
+
+    kwargs['use_reloader'] = True
+    kwargs['reloader_interval'] = 0.1
+    kwargs['reloader_type'] = %s
+    ''' % repr(reloader_type))
+    server.wait_for_reloader_loop()
+
+    connection = httplib.HTTPConnection(server.addr)
+    connection.request('GET', '/')
+    response = connection.getresponse()
+    assert response.status == 500
+
+    sub.write(textwrap.dedent('''
+    def real_app(environ, start_response):
+        start_response('200 OK', [('Content-Type', 'text/html')])
+        return [b'hello']
+    '''))
+    server.wait_for_reloader()
 
     connection.request('GET', '/')
     response = connection.getresponse()

tox.ini

@@ -9,6 +9,7 @@ deps=
     pytest
     pytest-xprocess
     redis
+    watchdog
 
 # Python 2
     py26: python-memcached

werkzeug/_reloader.py

@@ -40,16 +40,25 @@ def _recursive_walk(path_entry):
         entered.add(path_entry)
         try:
             for filename in os.listdir(path_entry):
-                if not filename.endswith(('.py', '.pyc', '.pyo')):
-                    continue
-                filename = _verify_file(os.path.join(path_entry, filename))
-                if filename:
-                    yield filename
+                path = os.path.join(path_entry, filename)
+                if os.path.isdir(path):
+                    for filename in _recursive_walk(path):
+                        yield filename
+                else:
+                    if not filename.endswith(('.py', '.pyc', '.pyo')):
+                        continue
+                    filename = _verify_file(path)
+                    if filename:
+                        yield filename
         except OSError:
             pass
 
     # The list call is necessary on Python 3 in case the module
     # dictionary modifies during iteration.
+    for path_entry in list(sys.path):
+        for filename in _recursive_walk(os.path.abspath(path_entry)):
+            yield filename
+
     for module in list(sys.modules.values()):
         if module is None:
             continue
@@ -65,7 +74,7 @@ def _recursive_walk(path_entry):
 
 def _find_observable_paths(extra_files=None):
     """Finds all paths that should be observed."""
-    rv = set()
+    rv = set(os.path.abspath(x) for x in sys.path)
     for filename in extra_files or ():
         rv.append(os.path.dirname(os.path.abspath(filename)))
     for module in list(sys.modules.values()):
@@ -98,6 +107,7 @@ def _walk(node, path):
 
 
 class ReloaderLoop(object):
+    _sleep = time.sleep  # monkeypatched by testsuite
     name = None
 
     def __init__(self, extra_files=None, interval=1):
@@ -154,7 +164,7 @@ def run(self):
                     continue
                 elif mtime > old_time:
                     self.trigger_reload(filename)
-            time.sleep(self.interval)
+            self._sleep(self.interval)
 
 
 class WatchdogReloaderLoop(ReloaderLoop):
@@ -168,7 +178,8 @@ def __init__(self, *args, **kwargs):
         def _check_modification(filename):
             if filename in self.extra_files:
                 self.trigger_reload(filename)
-            if os.path.dirname(filename) in self.observable_paths:
+            dirname = os.path.dirname(filename)
+            if dirname.startswith(tuple(self.observable_paths)):
                 if filename.endswith(('.pyc', '.pyo')):
                     self.trigger_reload(filename[:-1])
                 elif filename.endswith('.py'):
@@ -197,19 +208,25 @@ def run(self):
         def monitor_update_thread():
             while 1:
                 to_delete = set(watches)
-                paths = _find_observable_paths(self.extra_files)
-                common_roots = _find_common_roots(paths)
-                for path in common_roots:
+                paths = _find_common_roots(
+                    _find_observable_paths(self.extra_files))
+                for path in paths:
                     if path not in watches:
-                        watches[path] = observer.schedule(
-                            self.event_handler, path, recursive=True)
+                        try:
+                            watches[path] = observer.schedule(
+                                self.event_handler, path, recursive=True)
+                        except OSError:
+                            # "Path is not a directory". We could filter out
+                            # those paths beforehand, but that would cause
+                            # additional stat calls.
+                            watches[path] = None
                     to_delete.discard(path)
                 for path in to_delete:
                     watch = watches.pop(path, None)
                     if watch is not None:
                         observer.unschedule(watch)
                 self.observable_paths = paths
-                time.sleep(self.interval)
+                self._sleep(self.interval)
 
         t = threading.Thread(target=monitor_update_thread, args=())
         t.setDaemon(True)