-
Notifications
You must be signed in to change notification settings - Fork 46
SIGINT handler registration controller (supports deregister) #587
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
base: main
Are you sure you want to change the base?
Conversation
|
I've tested this with a local hf-xet build following the GH issue repro steps - and now CTRL+C correctly stops Uvicorn process on Windows. Will test on MacOS and then clean up the code before marking PR ready for review. |
|
OK tested on MacOS and see the expected CTRL+C behavior, when running Univorn after download is complete, hitting CTRL+C successfully exits the Python process. Also verified that while downloading model file pressing CTRL+C results in download cancelation. |
- attempts to call winapi directly as unsafe block to allow deregistering ctrl+c handler
6d69992 to
c88c73f
Compare
hoytak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't think registering and deregistering the signal handle is the correct approach for two reasons:
- There is a fundamental race condition here in that external_executor_count can be incremented at any point. As written, it can increment to one while we're deregistering the signal handler, which would leave it in a bad state. So it's problematic to do this.
- Registering a signal handler in the way we do it doesn't replace the previous ones, it just chains them. So the others get called. However, this is more subtle and here we find the root of the windows issues people discovered:
- On unix, all the installed signal handlers are called when they are registered using the current method. Since our signal handler does nothing but set an atomic, there's no reason to deregister it; the python or other signal handlers will get called immediately after that. That's one of the reasons it is so simple and all the logic is elsewhere.
- On windows, other signal handlers are also chained, but it's more subtle here as signal handlers are called in reverse order of installation until one returns true, which is the of the problem that we're trying to fix.
Digging into this a bit more, the ctrlc package signal handler returns true on windows, which blocks the other handers from being called. Thus the actual here fix is to change the windows handler to just set the SIGINT_DETECTED flag and always return false, allowing other handers to do the rest of the work there. We shouldn't have to change the
unix code at all.
- simply keep Windows CTRL_C SIGINT handling
Fixes #585
Coded with assistance from AI (Cursor/Composer).