Cleaner shutdown on Windows (#549)

* Work around signals on Windows (fixes #548)

As discussed in the issue above, this works around signals not working
properly on Windows by setting up an endpoint that shuts down the
server.

* Update requirements.txt

A few required packages were not named, and updated pytest.

* Fix _stop_dev_server when _session isn't set

* Get tests working

* Added CHANGES file

* Fixups for linter

* Skip a test on Python 3.7

* Delete 548.bugfix

* A workaround for issue #565

This change allows the tests to run multiple times without leaving
processes behind or leaving port 8000 open.

* Revert "Update requirements.txt"

This reverts commit 778ddbe.
This is now taken care of by my pull request #564.

* Apply a suggestion from code review

Co-authored-by: Sam Bull <[email protected]>

* Apply a suggestion from code review

Co-authored-by: Sam Bull <[email protected]>

* The issue #565 workaround needs to be adjusted

Because in this branch we've changed _stop_dev_server to be async

* Added a test for handlers

Tests whether on_startup, cleanup_ctx, and on_shutdown handlers are
called.

* Revert "A workaround for issue #565"

This reverts commit 11eab74.

* Fix #565

Co-authored-by: Sam Bull <[email protected]>

* Remove an unused function

* Turn off fail-fast on tests

* Hopefully make linter happy

* test_runserver_cleanup.py needs the #566 patch

* Copy over a filterwarnings to try and fix failures

* Fix requirements-dev.txt on Python 3.7

* Began work on test_server_cleanup_byurl (unfinished)

* Revert "Began work on test_server_cleanup_byurl (unfinished)"

This reverts commit 54eced8.

* Working shutdown/cleanup code

The manual test in cleanup_app.py passed on Windows 10, Python 3.7 through 3.11.

* The two tests marked non_windows_test now work

* Revert "Turn off fail-fast on tests"

This reverts commit 15317f3.

Not needed anymore now that all tests are passing

* Revert "Fix requirements-dev.txt on Python 3.7"

This reverts commit dcbe025.

PR #569 was rejected and I mainly needed it for testing.

* Make linter happy again

* Update aiohttp_devtools/runserver/watch.py

* Revert af852e0, thereby restoring 54eced8

This reverts commit af852e0,
which reverted commit 54eced8
"Began work on test_server_cleanup_byurl (unfinished)" because
it turns out we can use that code as a basis to test on Linux.

* Added --shutdown-by-url to CLI

* Replaced test_server_cleanup with _byurl version

* Update watch.py

* Update cli.py

* Update config.py

* Update serve.py

* Update cleanup_app.py

* Update test_runserver_cleanup.py

* Update test_runserver_cleanup.py

* Update watch.py

* Update cleanup_app.py

---------

Co-authored-by: Sam Bull <[email protected]>

Author: haukex

aiohttp_devtools/cli.py

@@ -46,6 +46,9 @@ def serve(path: str, livereload: bool, port: int, verbose: bool) -> None:
              'This can be used to watch a parent directory for changes in a more complex application. '
              'env variable: AIO_ROOT')
 static_url_help = 'URL path to serve static files from, default "/static/". env variable: AIO_STATIC_URL'
+shutdown_by_url_help = ("The development server will be stopped via a request to an endpoint that is "
+                        "added to the server, instead of via signals (this is the default on Windows). "
+                        "env variable: AIO_SHUTDOWN_BY_URL")
 host_help = ('host used when referencing livereload and static files, if blank host is taken from the request header '
              'with default of localhost. env variable AIO_HOST')
 app_factory_help = ('name of the app factory to create an aiohttp.web.Application with, if missing default app-factory '
@@ -62,6 +65,8 @@ def serve(path: str, livereload: bool, port: int, verbose: bool) -> None:
 @click.option('-s', '--static', 'static_path', envvar='AIO_STATIC_PATH', type=_dir_existing, help=static_help)
 @click.option('--root', 'root_path', envvar='AIO_ROOT', type=_dir_existing, help=root_help)
 @click.option('--static-url', envvar='AIO_STATIC_URL', help=static_url_help)
+@click.option("--shutdown-by-url/--no-shutdown-by-url", default=sys.platform.startswith("win32"),
+              envvar="AIO_SHUTDOWN_BY_URL", help=shutdown_by_url_help)
 @click.option('--livereload/--no-livereload', envvar='AIO_LIVERELOAD', default=None, help=livereload_help)
 @click.option('--host', default=INFER_HOST, help=host_help)
 @click.option('--app-factory', 'app_factory_name', envvar='AIO_APP_FACTORY', help=app_factory_help)

aiohttp_devtools/runserver/config.py

@@ -38,6 +38,8 @@ def __init__(self, *,
                  python_path: Optional[str] = None,
                  static_url: str = '/static/',
                  livereload: bool = True,
+                 shutdown_by_url: bool = sys.platform.startswith("win32"),
+                 path_prefix: str = "/_devtools",
                  app_factory_name: Optional[str] = None,
                  host: str = INFER_HOST,
                  main_port: int = 8000,
@@ -66,6 +68,8 @@ def __init__(self, *,
         self.static_path = self._resolve_path(static_path, "is_dir", "static-path") if static_path else None
         self.static_url = static_url
         self.livereload = livereload
+        self.shutdown_by_url = shutdown_by_url
+        self.path_prefix = path_prefix
         self.app_factory_name = app_factory_name
         self.infer_host = host == INFER_HOST
         self.host = 'localhost' if self.infer_host else host
@@ -183,6 +187,6 @@ async def load_app(self, app_factory: AppFactory) -> web.Application:
         return app
 
     def __str__(self) -> str:
-        fields = ('py_file', 'static_path', 'static_url', 'livereload',
-                  'app_factory_name', 'host', 'main_port', 'aux_port')
+        fields = ("py_file", "static_path", "static_url", "livereload", "shutdown_by_url",
+                  "path_prefix", "app_factory_name", "host", "main_port", "aux_port")
         return 'Config:\n' + '\n'.join('  {0}: {1!r}'.format(f, getattr(self, f)) for f in fields)

aiohttp_devtools/runserver/serve.py

@@ -5,12 +5,13 @@
 import sys
 from errno import EADDRINUSE
 from pathlib import Path
-from typing import Any, Iterator, Optional
+from typing import Any, Iterator, Optional, NoReturn
 
 from aiohttp import WSMsgType, web
 from aiohttp.hdrs import LAST_MODIFIED, CONTENT_LENGTH
 from aiohttp.typedefs import Handler
 from aiohttp.web_exceptions import HTTPNotFound, HTTPNotModified
+from aiohttp.web_runner import GracefulExit
 from aiohttp.web_urldispatcher import StaticResource
 from yarl import URL
 
@@ -39,7 +40,7 @@ def _change_static_url(app: web.Application, url: str) -> None:
         _change_static_url(subapp, url)
 
 
-def modify_main_app(app: web.Application, config: Config) -> None:
+def modify_main_app(app: web.Application, config: Config) -> None:  # noqa: C901
     """
     Modify the app we're serving to make development easier, eg.
     * modify responses to add the livereload snippet
@@ -79,6 +80,18 @@ async def static_middleware(request: web.Request, handler: Handler) -> web.Strea
 
         app.middlewares.insert(0, static_middleware)
 
+    # Fallback option to shutdown the application if signals don't work (e.g. Windows).
+    if config.shutdown_by_url:
+        async def do_shutdown(request: web.Request) -> web.Response:
+            def shutdown() -> NoReturn:
+                raise GracefulExit()
+            asyncio.get_running_loop().call_soon(shutdown)
+            return web.Response()
+
+        path = config.path_prefix + "/shutdown"
+        app.router.add_route("GET", path, do_shutdown, name="_devtools.shutdown")
+        dft_logger.debug("Created shutdown endpoint at http://{}:{}{}".format(config.host, config.main_port, path))
+
     if config.static_path is not None:
         static_url = 'http://{}:{}/{}'.format(config.host, config.aux_port, static_path)
         dft_logger.debug('settings app static_root_url to "%s"', static_url)

aiohttp_devtools/runserver/watch.py

@@ -5,8 +5,10 @@
 from multiprocessing import Process
 from pathlib import Path
 from typing import AsyncIterator, Iterable, Optional, Tuple, Union
+from contextlib import suppress
 
 from aiohttp import ClientSession, web
+from aiohttp.client_exceptions import ClientError, ClientConnectionError
 from watchfiles import awatch
 
 from ..exceptions import AiohttpDevException
@@ -71,7 +73,7 @@ def is_static(changes: Iterable[Tuple[object, str]]) -> bool:
                 self._reloads += 1
                 if any(f.endswith('.py') for _, f in changes):
                     logger.debug('%d changes, restarting server', len(changes))
-                    self._stop_dev_server()
+                    await self._stop_dev_server()
                     self._start_dev_server()
                     await self._src_reload_when_live(live_checks)
                 elif len(changes) == 1 and is_static(changes):
@@ -119,10 +121,34 @@ def _start_dev_server(self) -> None:
         self._process = Process(target=serve_main_app, args=(self._config, tty_path))
         self._process.start()
 
-    def _stop_dev_server(self) -> None:
+    async def _stop_dev_server(self) -> None:
         if self._process.is_alive():
             logger.debug('stopping server process...')
+            if self._config.shutdown_by_url:  # Workaround for signals not working on Windows
+                url = "http://localhost:{}{}/shutdown".format(self._config.main_port, self._config.path_prefix)
+                logger.debug("Attempting to stop process via shutdown endpoint {}".format(url))
+                try:
+                    with suppress(ClientConnectionError):
+                        async with ClientSession() as session:
+                            async with session.get(url):
+                                pass
+                except (ConnectionError, ClientError, asyncio.TimeoutError) as ex:
+                    if self._process.is_alive():
+                        msg = "shutdown endpoint caused an error (will try signals next)"
+                        logger.warning(msg.format(type(ex), ex), exc_info=True)
+                    else:
+                        msg = "process stopped (despite error at shutdown endpoint)"
+                        logger.warning(msg.format(type(ex), ex), exc_info=True)
+                        return
+                else:
+                    self._process.join(5)
+                    if self._process.exitcode is None:
+                        logger.warning("shutdown endpoint did not terminate process, trying signals")
+                    else:
+                        logger.debug("process stopped via shutdown endpoint")
+                        return
             if self._process.pid:
+                logger.debug("sending SIGINT")
                 os.kill(self._process.pid, signal.SIGINT)
             self._process.join(5)
             if self._process.exitcode is None:
@@ -136,7 +162,7 @@ def _stop_dev_server(self) -> None:
 
     async def close(self, *args: object) -> None:
         self.stopper.set()
-        self._stop_dev_server()
+        await self._stop_dev_server()
         if self._session is None:
             raise RuntimeError("Object not started correctly before calling .close()")
         await asyncio.gather(super().close(), self._session.close())

tests/cleanup_app.py

@@ -0,0 +1,45 @@
+"""
+Test script for checking if cleanup/shutdown handlers are called.
+Used in test_runserver_cleanup.py.
+
+This test has proved to be quite difficult to automate in Windows
+(see discussion at https://github.com/aio-libs/aiohttp-devtools/pull/549)
+so it must be done manually as per the protocol below.
+
+Test Protocol:
+
+1. Run a Windows console such as Git Bash; ensure a working Python version is available
+2. ``cd`` to the main aiohttp-devtools directory (root of the git repo)
+3. Run ``python -c "from aiohttp_devtools.cli import runserver; runserver()" -v tests/cleanup_app.py``
+4. The console output should show the "CTX BEFORE" and "STARTUP" output from the code below
+5. Edit this file, e.g. a simple whitespace change, and save
+6. The console output should show the "SHUTDOWN" and "CTX AFTER" output, followed by the startup output
+7. End the process in the console with Ctrl-C
+8. The console should again show the shutdown output
+"""
+from aiohttp import web
+
+
+async def hello(_request):
+    return web.Response(text="hello, world")
+
+
+async def startup(_app):
+    print("====> STARTUP")
+
+
+async def context(_app):
+    print("====> CTX BEFORE")
+    yield
+    print("====> CTX AFTER")
+
+
+async def shutdown(_app):
+    print("====> SHUTDOWN")
+
+
+app = web.Application()
+app.router.add_get("/", hello)
+app.on_startup.append(startup)
+app.cleanup_ctx.append(context)
+app.on_shutdown.append(shutdown)

tests/test_runserver_cleanup.py

@@ -0,0 +1,42 @@
+import asyncio
+import sys
+from pathlib import Path
+
+from aiohttp import ClientSession, ClientTimeout
+
+from .conftest import forked
+
+
+@forked  # forked doesn't run on Windows and is skipped - see cleanup_app.py instead
+async def test_server_cleanup_by_url() -> None:
+    async def is_server_running() -> bool:
+        async with ClientSession(timeout=ClientTimeout(total=1)) as session:
+            for i in range(30):
+                try:
+                    async with session.get("http://localhost:8000/") as r:
+                        assert r.status == 200
+                        text = await r.text()
+                        assert "hello, world" in text
+                        return True
+                except OSError:
+                    await asyncio.sleep(0.5)
+                except asyncio.TimeoutError:
+                    pass
+        return False
+
+    proc = await asyncio.create_subprocess_exec(
+        sys.executable, "-c", "from aiohttp_devtools.cli import runserver; runserver()",  # same as `adev runserver`
+        str(Path(__file__).parent / "cleanup_app.py"), "--shutdown-by-url", "-v",  # TODO(PY38): Remove str()
+        stdout=asyncio.subprocess.PIPE,
+        stderr=asyncio.subprocess.PIPE,
+    )
+    try:
+        assert await is_server_running()
+    finally:
+        proc.terminate()
+
+    await asyncio.sleep(2)
+    stdout, stderr = await proc.communicate()
+    assert b"process stopped via shutdown endpoint" in stderr
+    lines = tuple(x[6:] for x in stdout.decode('UTF-8').splitlines() if x.startswith('====> '))
+    assert lines == ("CTX BEFORE", "STARTUP", "SHUTDOWN", "CTX AFTER")

tests/test_runserver_main.py

@@ -1,8 +1,5 @@
 import asyncio
 import json
-import os
-import signal
-import time
 from unittest import mock
 
 import aiohttp
@@ -115,11 +112,6 @@ async def hello(request):
     assert len(aux_app.cleanup_ctx) == 1
 
 
-def kill_parent_soon(pid):
-    time.sleep(0.2)
-    os.kill(pid, signal.SIGINT)
-
-
 @forked
 async def test_run_app_aiohttp_client(tmpworkdir, aiohttp_client):
     mktree(tmpworkdir, SIMPLE_APP)

tests/test_runserver_serve.py

@@ -1,7 +1,6 @@
 import json
 import pathlib
 import socket
-from platform import system as get_os_family
 from typing import Dict
 from unittest.mock import MagicMock
 
@@ -16,18 +15,12 @@
 
 from .conftest import SIMPLE_APP, create_future
 
-non_windows_test = pytest.mark.skipif(
-    get_os_family() == 'Windows',
-    reason='This only works under UNIX-based OS and gets stuck under Windows',
-)
-
 
 async def test_check_port_open(unused_tcp_port_factory):
     port = unused_tcp_port_factory()
     await check_port_open(port, 0.001)
 
 
-@non_windows_test  # FIXME: probably needs some sock options
 async def test_check_port_not_open(unused_tcp_port_factory):
     port = unused_tcp_port_factory()
     with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:

tests/test_runserver_watch.py

@@ -1,6 +1,6 @@
+import sys
 import asyncio
 from functools import partial
-from platform import system as get_os_family
 from unittest.mock import MagicMock, call
 
 import pytest
@@ -11,9 +11,10 @@
 
 from .conftest import create_future
 
-non_windows_test = pytest.mark.skipif(
-    get_os_family() == 'Windows',
-    reason='This only works under UNIX-based OS and gets stuck under Windows',
+
+needs_py38_test = pytest.mark.skipif(
+    sys.version_info < (3, 8),
+    reason="This only works on Python >=3.8 because otherwise MagicMock can't be used in 'await' expression",
 )
 
 
@@ -75,7 +76,7 @@ async def test_multiple_file_change(event_loop, mocker):
     await app_task._session.close()
 
 
-@non_windows_test
+@needs_py38_test
 async def test_python_no_server(event_loop, mocker):
     mocked_awatch = mocker.patch('aiohttp_devtools.runserver.watch.awatch')
     mocked_awatch.side_effect = create_awatch_mock({('x', '/path/to/file.py')})
@@ -159,20 +160,20 @@ def join(self, wait):
         pass
 
 
-def test_stop_process_dead(smart_caplog, mocker):
+async def test_stop_process_dead(smart_caplog, mocker):
     mock_kill = mocker.patch('aiohttp_devtools.runserver.watch.os.kill')
     mocker.patch('aiohttp_devtools.runserver.watch.awatch')
     mocker.patch('asyncio.Event')
     app_task = AppTask(MagicMock())
     app_task._process = MagicMock()
     app_task._process.is_alive = MagicMock(return_value=False)
     app_task._process.exitcode = 123
-    app_task._stop_dev_server()
+    await app_task._stop_dev_server()
     assert 'server process already dead, exit code: 123' in smart_caplog
     assert mock_kill.called is False
 
 
-def test_stop_process_clean(mocker):
+async def test_stop_process_clean(mocker):
     mock_kill = mocker.patch('aiohttp_devtools.runserver.watch.os.kill')
     mocker.patch('aiohttp_devtools.runserver.watch.awatch')
     mocker.patch('asyncio.Event')
@@ -181,11 +182,10 @@ def test_stop_process_clean(mocker):
     app_task._process.is_alive = MagicMock(return_value=True)
     app_task._process.pid = 321
     app_task._process.exitcode = 123
-    app_task._stop_dev_server()
+    await app_task._stop_dev_server()
     assert mock_kill.called_once_with(321, 2)
 
 
-@non_windows_test  # There's no signals in Windows
 async def test_stop_process_dirty(mocker):
     mock_kill = mocker.patch('aiohttp_devtools.runserver.watch.os.kill')
     mocker.patch('aiohttp_devtools.runserver.watch.awatch')
@@ -195,6 +195,6 @@ async def test_stop_process_dirty(mocker):
     process_mock.is_alive = MagicMock(return_value=True)
     process_mock.pid = 321
     process_mock.exitcode = None
-    app_task._stop_dev_server()
+    await app_task._stop_dev_server()
     assert mock_kill.call_args_list == [call(321, 2)]
     assert process_mock.kill.called_once()