Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle SIGBREAK for Windows #1909

Merged
merged 4 commits into from
Apr 15, 2023
Merged

Handle SIGBREAK for Windows #1909

merged 4 commits into from
Apr 15, 2023

Conversation

yanyongyu
Copy link
Contributor

On Windows, subprocess.CTRL_BREAK_EVENT may be sent to subprocess using os.kill or process.send_signal as SIGBREAK signal. This signal can be catched by subprocess to do a graceful shutdown instead of terminating the subprocess directly.
And, this signal is also catched in hypercorn.

@Kludex
Copy link
Member

Kludex commented Mar 23, 2023

Is this an alternative to #1584 ?

@yanyongyu
Copy link
Contributor Author

yanyongyu commented Mar 23, 2023

Is this an alternative to #1584 ?

This pr may be related to #1584 but not an alternative. The reloader should do something else on Windows including send CTRL_BREAK_EVENT(or maybe CTRL_C_EVENT) to worker process and preventing SIGBREAK(or CTRL_C_EVENT) being handled in main process when reloading.

In our application, we use subprocess to run uvicorn now and SIGBREAK is sent to subprocess on Windows when shutting down (as process.terminate will call winapi to force exit). uvicorn uses multiprocessing to spawn workers, i'm not sure if the signal handling is the same. maybe @StarHeartHunt we need a further testing on uvicorn reloader.

Another problem is that CTRL_C_EVENT will be sent to all processes in the process group on Windows. We cannot use it as we have multiple subprocess workers running. We finally change to using subprocess.CREATE_NEW_PROCESS_GROUP and signal.CTRL_BREAK_EVENT.😓

Reference:

  1. note in python docs: https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
  2. our code: https://github.com/nonebot/nb-cli/blob/0221588fdc132891fbe7ccd8a8b15487380e722c/nb_cli/handlers/process.py#L56-L104
  3. windows docs: https://learn.microsoft.com/en-us/windows/console/generateconsolectrlevent, https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags

@StarHeartHunt
Copy link
Contributor

StarHeartHunt commented Mar 23, 2023

As mentioned above, The Ctrl + Break signal is supposed to be handled by uvicorn as a kind of exit signal too, for external subprocess calls with subprocess.CREATE_NEW_PROCESS_GROUP to gracefully shutdown the server process, This is what this PR meant for.

And #1584 tries fixing the graceful exit in reloader when uvicorn it self is the root process, it seems in this case we can use Ctrl+C signal to achieve this goal (Maybe the internal code is different in multiprocessing?). The solution has been proved to be valid on my Windows 10 laptop.

TL;DR: Based on current investigation and discussions in our team @nonebot, To implement a graceful exit:

  1. subprocess with a subprocess.CREATE_NEW_PROCESS_GROUP flag or multiprocessing
  2. The main process can only use CTRL_BREAK_EVENT as exit signal in subprocess situations, but multiprocessing could use CTRL_C_EVENT, and in both situations, the main process should implement a shield context to avoid being terminated by itself.
  3. A signal (CTRL_BREAK_EVENT / CTRL_C_EVENT) is sent to the subprocess.

uvicorn/server.py Outdated Show resolved Hide resolved
@Kludex Kludex added this to the Version 0.22.0 milestone Mar 24, 2023
@Kludex Kludex added the windows label Mar 24, 2023
@Kludex
Copy link
Member

Kludex commented Mar 24, 2023

Can you also show me the reference on hypercorn that you mentioned in the description, please?

@yanyongyu
Copy link
Contributor Author

Can you also show me the reference on hypercorn that you mentioned in the description, please?

That's here: https://github.com/pgjones/hypercorn/blob/8ae17ca68204d9718389fb3649ca0ed6ba851906/src/hypercorn/run.py#L61-L63

By the way, hypercorn uses a multiprocessing spawn Event instead of signals to notify the worker to exit.

uvicorn/server.py Outdated Show resolved Hide resolved
@Kludex Kludex merged commit 411dd87 into encode:master Apr 15, 2023
@yanyongyu yanyongyu deleted the patch-1 branch April 15, 2023 14:53
@Kludex
Copy link
Member

Kludex commented May 13, 2023

Did this caused #1972 ?

@yanyongyu
Copy link
Contributor Author

Our team member test the 0.22.0 version but can not repreduce the issue on windows 10. The detailed info is shown in the #1584 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants