Fix shutdown event on Windows in reloader

[StarHeartHunt]

On Windows, using Process.terminate() will immediately kill the child process so that the asgi event shutdown cannot be triggered in reloader. This PR is going to implement a graceful exit of child process.

[Kludex]

For reference, what the description says is written here: https://docs.python.org/3/library/multiprocessing.html#multiprocessing.Process.terminate

[Kludex]

Without the reload flag, does it run the shutdown event?

[StarHeartHunt]

Without the reload flag, does it run the shutdown event?

In my tests, It seems the shutdown event is set without the reload flag, since there is no SpawnProcess.terminate() in the exit signal handler of main process

[Kludex]

Can we do something like https://stackoverflow.com/a/47314406/13087720 here?

[StarHeartHunt]

Can we do something like https://stackoverflow.com/a/47314406/13087720 here?

After digging into multiprocessing module, I found that it passes a 0 to _winapi.CreateProcess as creationflags value and can't be modified outside.

https://github.com/python/cpython/blob/3.10/Lib/multiprocessing/popen_spawn_win32.py#L70-L79

        with open(wfd, 'wb', closefd=True) as to_child:
            # start process
            try:
                hp, ht, pid, tid = _winapi.CreateProcess(
                    python_exe, cmd,
                    None, None, False, 0, env, None, None)
                _winapi.CloseHandle(ht)
            except:
                _winapi.CloseHandle(rhandle)
                raise

While subprocess seems only have support for command executions (https://stackoverflow.com/questions/2046603/is-it-possible-to-run-function-in-a-subprocess-without-threading-or-writing-a-se), it's quite hard to use it instead.

[Kludex]

@StarHeartHunt If you have time, it would be cool to add a test for the changes, and fix the pipeline. 👀 🙏

[Kludex]

A test is still needed for Windows (pytest.mark.skipif should help), and I need some people with Windows to help me here. 👀

[iudeen]

@Kludex i have windows! :)

[Kludex]

Ok... And this PR works there? 👀

[StarHeartHunt]

A test is still needed for Windows (pytest.mark.skipif should help), and I need some people with Windows to help me here. 👀

Tried getting a subprocess in pytest but got an EOFError: run out of input. I need some help from people familiar with writing test scripts on cross-module cases like this.👐

[Kludex]

A test is still needed for Windows (pytest.mark.skipif should help), and I need some people with Windows to help me here. eyes

Tried getting a subprocess in pytest but got an EOFError: run out of input. I need some help from people familiar with writing test scripts on cross-module cases like this.open_hands

We actually already have a test for it, it's the one you've modified 😅

So... Someone can confirm this works? Someone from the team has a way to test this locally? @encode/maintainers

[GuillaumeDesforges]

pytest works on my Windows 11 machine

EDIT: I meant all tests pass

[Kludex]

pytest works on my Windows 11 machine

EDIT: I meant all tests pass

By "testing" I mean manually checking that it works on Windows. We have windows on our pipeline, so I know the tests are passing on Windows 👀

[GuillaumeDesforges]

Works on my machine

app.py

from contextlib import asynccontextmanager
from typing import AsyncGenerator
from fastapi import FastAPI
from starlette.applications import Starlette


@asynccontextmanager
async def lifespan(app: Starlette) -> AsyncGenerator[None, None]:
    print("START")
    yield None
    print("SHUTDOWN")


app = FastAPI(lifespan=lifespan)

Install deps

$ pip install fastapi
$ pip install git+https://github.com/StarHeartHunt/uvicorn@bug/fix-shutdown-event-on-windows-in-hot-reloader --force-reinstall

Run

$ uvicorn app:app --reload
INFO:     Will watch for changes in these directories: ['C:\\Users\\aceus\\perso\\tmp']
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     Started reloader process [2108] using StatReload
INFO:     Started server process [19776]
INFO:     Waiting for application startup.
START
INFO:     Application startup complete.
WARNING:  StatReload detected changes in 'app.py'. Reloading...
INFO:     Shutting down
INFO:     Waiting for application shutdown.
SHUTDOWN
INFO:     Application shutdown complete.
INFO:     Finished server process [19776]
INFO:     Started server process [8672]
INFO:     Waiting for application startup.
START
INFO:     Application startup complete.
INFO:     Shutting down
INFO:     Waiting for application shutdown.
SHUTDOWN
INFO:     Application shutdown complete.
INFO:     Finished server process [8672]
INFO:     Stopping reloader process [2108]

[yanyongyu]

Hi, this pr works with our application and the log is here: nonebot/nonebot2#1654 (comment)

[Kludex]

Thanks @StarHeartHunt 🙏

[Kludex]

Do we need to do the same for the Multiprocess class?

[StarHeartHunt]

Do we need to do the same for the Multiprocess class?

I think there is no need to apply this patch to Multiprocess class. Maybe #1872 is relevant to #1909, we can have a further triage after this PR is merged👀

[abersheeran]

I'm not sure if this PR will meet the issues in #684. I can't test because I'm using a MacBook right now, it would be nice if someone could try it.

[musale]

I'm not sure if this PR will meet the issues in #684. I can't test because I'm using a MacBook right now, it would be nice if someone could try it.

Using the repro by @abersheeran in it, I tried, and it works well.

➜ uvicorn app:app --reload
INFO:     Will watch for changes in these directories: ['C:\\Users\\ava\\Projects\\uvicorn-test']
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     Started reloader process [2704] using StatReload
INFO:     Started server process [12708]
INFO:     Waiting for application startup.
START
ERROR:    Traceback (most recent call last):
  File "C:\Users\ava\AppData\Local\Programs\Python\Python312\Lib\site-packages\starlette\routing.py", line 677, in lifespan
    async with self.lifespan_context(app) as maybe_state:
  File "C:\Users\ava\AppData\Local\Programs\Python\Python312\Lib\contextlib.py", line 204, in __aenter__
    return await anext(self.gen)
           ^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\ava\Projects\uvicorn-test\app.py", line 10, in lifespan
    afhajdhfa
NameError: name 'afhajdhfa' is not defined

ERROR:    Application startup failed. Exiting.
WARNING:  StatReload detected changes in 'app.py'. Reloading...
INFO:     Started server process [11988]
INFO:     Waiting for application startup.
START
INFO:     Application startup complete.

[abersheeran]

I'm not sure if this PR will meet the issues in #684. I can't test because I'm using a MacBook right now, it would be nice if someone could try it.

Using the repro by @abersheeran in it, I tried, and it works well.

➜ uvicorn app:app --reload
INFO:     Will watch for changes in these directories: ['C:\\Users\\ava\\Projects\\uvicorn-test']
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     Started reloader process [2704] using StatReload
INFO:     Started server process [12708]
INFO:     Waiting for application startup.
START
ERROR:    Traceback (most recent call last):
  File "C:\Users\ava\AppData\Local\Programs\Python\Python312\Lib\site-packages\starlette\routing.py", line 677, in lifespan
    async with self.lifespan_context(app) as maybe_state:
  File "C:\Users\ava\AppData\Local\Programs\Python\Python312\Lib\contextlib.py", line 204, in __aenter__
    return await anext(self.gen)
           ^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\ava\Projects\uvicorn-test\app.py", line 10, in lifespan
    afhajdhfa
NameError: name 'afhajdhfa' is not defined

ERROR:    Application startup failed. Exiting.
WARNING:  StatReload detected changes in 'app.py'. Reloading...
INFO:     Started server process [11988]
INFO:     Waiting for application startup.
START
INFO:     Application startup complete.

Thanks.

[Kludex]

Once rebased, we can merge it. 🙏

Once again, thanks @abersheeran 🙏

[Kludex]

Did this caused #1977 ?

[StarHeartHunt]

Did this caused #1977 ?

It seems couldn't be reproduced on my Windows 10 laptop. I think it's not relevant to #1909 or #1584

Use case and test script, by randomly adding and removing lines to trigger reload and see

from contextlib import asynccontextmanager
from typing import Union

from fastapi import FastAPI

@asynccontextmanager
async def lifespan(app: FastAPI):
    print("startup")
    yield
    print("shutdown")

app = FastAPI(lifespan=lifespan)

@app.get("/")
def read_root():
    return {"Hello": "World"}

@app.get("/items/{item_id}")
def read_item(item_id: int, q: Union[str, None] = None):
    return {"item_id": item_id, "q": q}

Start uvicorn by the following command

uvicorn main:app --reload

And the environment

PS D:\PythonWorkspace\reloadtest> pip list                 
Package            Version   Editable project location
------------------ --------- ----------------------------------
1223               0.0.1     D:\PythonWorkspace\reloadtest     
anyio              3.6.2
arrow              1.2.3
asds               0.1.0     D:\PythonWorkspace\reloadtest\asds
binaryornot        0.4.4
cashews            5.3.1
certifi            2022.12.7
chardet            5.1.0
charset-normalizer 3.1.0
click              8.1.3
colorama           0.4.6
cookiecutter       2.1.1
distlib            0.3.6
fastapi            0.95.0
filelock           3.10.3
h11                0.14.0
httpcore           0.16.3
httpx              0.23.3
idna               3.4
importlib-metadata 6.1.0
Jinja2             3.1.2
jinja2-time        0.2.0
loguru             0.6.0
MarkupSafe         2.1.2
multidict          6.0.4
nb-cli             1.0.5
nonebot2           2.0.0rc3
noneprompt         0.1.9
pip                23.0.1
platformdirs       2.6.2
prompt-toolkit     3.0.38
pydantic           1.10.7
pyfiglet           0.8.post1
pygtrie            2.5.0
python-dateutil    2.8.2
python-dotenv      1.0.0
python-slugify     8.0.1
PyYAML             6.0
requests           2.28.2
rfc3986            1.5.0
setuptools         67.6.0
six                1.16.0
sniffio            1.3.0
starlette          0.26.1
text-unidecode     1.3
typing_extensions  4.5.0
urllib3            1.26.15
uvicorn            0.22.0
virtualenv         20.17.1
watchfiles         0.18.1
wcwidth            0.2.6
win32-setctime     1.1.0
yarl               1.8.2
zipp               3.15.0