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

awatch on windows can't be killed with Ctrl+C #110

Closed
samuelcolvin opened this issue Mar 24, 2022 · 8 comments · Fixed by #113
Closed

awatch on windows can't be killed with Ctrl+C #110

samuelcolvin opened this issue Mar 24, 2022 · 8 comments · Fixed by #113
Labels

Comments

@samuelcolvin
Copy link
Owner

samuelcolvin commented Mar 24, 2022

The "KeyboardInterrupt" gets logged but the process doesn't terminate until the next file system event happens and RustNotify.watch() returns.

This is because windows doesn't have signals, so open_signal_receiver

with anyio.open_signal_receiver(signal.SIGINT) as signals:

Doesn't work and there's no way to tell the rust code to terminate.

Potential solution is to return from RustNotify.watch() regularly, and thereby check if an error has been raised. Seems a bit ugly but I can't think of a better solution.

Let's see if people actually suffer from this?

samuelcolvin added a commit that referenced this issue Apr 2, 2022
samuelcolvin added a commit that referenced this issue Apr 8, 2022
* allow timeout in rust, fix #110

* fix timeout logic

* tests for watch and awatch on timeout

* add "yield_on_timeout"

* fix Makefile

* tweak stop event logic

* tweak logic for event.is_set, tests
@TBBle
Copy link
Contributor

TBBle commented May 7, 2022

I just came across this older bug, and I'm trying to wrap my head around it: SIGINT does work on Windows (but won't interrupt the thread's blocking API call, AFAIK), and AFAIK the call to PyErr_CheckSignals (py.check_signals() in this codebase) done each step should have triggered a KeyboardInterrupt in the main thread when it was called. The fact that a KeyboardInterrupt was seen says that SIGINT was called, as that is done by python's default SIGINT handler in Python code.

I do wonder if PyErr_CheckSignals called on a thread doesn't raise an error on Windows if the signal's already been handled on the main thread? (Edit: PyErr_CheckSignals on a thread is a no-op on all platforms) But the stop_event should have been set and hence terminated the loop in the next step if that was the case. So I'm somewhat mystified here.

Note: I got interested in this partly because I was looking at setting rust_timeout to 0 and the docs linked to this bug-report, and because I was having a lot of trouble with Control-C handling, seeming to get two KeyboardInterrupt exceptions (one during cleanup after handling the other to terminate my program), until I added exit_on_signal=False to my awatch call. I suspect the logic around signals in the code here goes awry when run on a thread, since Python is already injecting a KeyboardInterrupt into the main thread, and this might have muddied the waters on the Windows-side of the issue further.

At some point, I might try and raise a separate bug report about this 'Note', if I can make it happen in isolation.

@samuelcolvin
Copy link
Owner Author

Does sound mystifying. I don't have windows and your logic makes sense, so I don't know what to suggest.

@TBBle
Copy link
Contributor

TBBle commented May 7, 2022

Fair 'nuff. I haven't tested my app directly on Windows, since we're running it inside a container. If I have some spare time, I'll have a play with Windows behaviours when I'm looking into the double-KeyboardInterrupt weirdness.

@samuelcolvin
Copy link
Owner Author

When I tested it on a VM on Ubuntu it seemed it to work. The stop_event didn't work, but the exception propagated on the next return from the rust code.

But I might have missed something.

@TBBle
Copy link
Contributor

TBBle commented May 7, 2022

Hmm. I noticed the comment added more-recently, and it does indeed appear that asyncio.add_signal_handler doesn't work on Windows, although there's no clear indication why, since the only Unix-only API it calls is already in that state on Windows, and e.g., signal.signal is supported on Windows.

So yeah, the original issue description was give-or-take correct, anyio.open_signal_receiver doesn't work because asyncio.add_signal_handler is not implemented on Windows, rather than lack of signals at all.

@samuelcolvin
Copy link
Owner Author

Also worth checking #136 to see if that makes it better or worse

@TBBle
Copy link
Contributor

TBBle commented May 7, 2022

Oh, interesting, the changes in #136 look like they are very similar to what I did to avoid the double-KeyboardInterrupt, by catching the cancelation of the awaited task, and setting my own stop_event when I saw it, to terminate the background threaded watcher. Looking at #128 (comment), that looks like roughly the same double-KeyboardInterrupt issue I saw.

So I'll follow up on that part of it in #128 if I come back to it, and for the actual topic of this bug, it looks like it's an asyncio deficiency that'll be hard to work around.

@Luiz-Monad
Copy link

Using an Event seems like the way to do it. https://github.com/ThomasMonkman/filewatch/blob/a59891baf375b73ff28144973a6fafd3fe40aa21/FileWatch.hpp#L330

Windows has IOCP, which is IO completion ports which can do OVERLAPPED I/O.
I'm mystified python doesn't provide a signaling implementation using events.
And you can cancel I/O on Windows using that if you start them as Async.

One would presume "async" I/O uses actual asynchronous APIs from the operating system instead of a separate thread, I guess its some python limitation.

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 a pull request may close this issue.

3 participants