-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Qt5: SIGINT kills just the mpl window and not the process itself #13306
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
Conversation
I'd downvote. Timers kill performance, they're an exceptionally bad idea. If you really want Python to handle a signal, you should register a handler with Qt: http://doc.qt.io/qt-5/unix-signals.html. (There is an analogue Windows method to this as well.) There is some discussion as to whether this is a good idea or not: #13302 (comment) |
Removed QTimer, using KeyboardInterrupt is raised after |
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.
Also, I think Windows testing is important. Windows does sockets differently than Unix and I don't know what the Windows gotchas are. Also, your ctrl-c handlers for sure only get handled once the app processes some Python in Windows. If there's a path of execution through the event loop that stays in C, the Python callback writing to the socket will never happen. Windows really does need to have that callback triggered by its own ctrl-c handler a la https://github.com/zeromq/pyzmq/blob/39e679937bcedb178860847e81e931cda1b534a0/zmq/utils/win32.py#L116.
Current approach seems to work on Windows almost out of the box, only had to change the socket type to default (STREAM) since Windows didn't allow DGRAM. The test however now doesn't work since os.kill(os.getpid(), signal.SIGINT) kills CPython with pytest and everything. I again faced (and captured) the bug which is about |
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.
I think this can be done more simply, I want to have final review on this.
@tacaswell, it's certainly not ready yet, I need to test more and try to simplify =) @joelfrederico, if I understand everything correctly, Python registers some low-level handler for SIGINTS that is actually setting a flag for the VM and writing to the file descriptor according to I also think I grasped what is in your zmq link with kernel32.SetConsoleCtrlHandler, I think it's also a way to try. |
Sorry, messed up with commits and the author names a bit, it was all me. I've implemented the context manager (thanks, @joelfrederico), fixed the tests. I think I am starting to like this code, but now I get |
So... yes, Python actually has a SIGINT handler that, in an undocumented manner, records that a SIGINT has occurred. That's all the handler does. Then, before every python instruction, python checks to see if a SIGINT has occurred. If it has, the python code runs the current Python-registered signal handler function. But what actually happens if you're in the C part of Qt when a signal happens? The handler records that a SIGINT has occurred and returns. That's it. The Python-registered signal handler function will happen the next time any Python code runs. But what happens if no Python code ever runs again? Qt will continue along as if nothing happened. It will never call the Python-registered signal handler function, so there will be no socket event, so it will never quit. Instead, what we probably want to happen is to register our own low-level signal handler. Then, if a signal happens during Qt C, our low-level signal handler runs. It then writes to a socket and returns. Our Qt should have a QSocketNotifier listening on that socket. Once the Qt event loops gets around to checking that socket, it sees the write and then emits a signal that it has detected an interrupt. Up until this point, everything is in C, no Python. The question is, what do we want the SIGINT slots to do? We have options. I think a solid option is to simply call the custom handler (that is effectively a closure) that you designed. That custom handler records that a SIGINT has happened, followed by a call to Qt |
The astute observer will notice the problem: what happens with us overriding Python's SIGINT handler? Does it get reset at some point, or does ours take precedence? If so, we need to make sure it gets reset. I don't really know how to do this. It should be possible in theory, just a question of whether Python's low-level SIGINT handler is documented. Like I said, I switched away from dealing with this, so I didn't tackle this issue. |
Here I can vaguely see that the handler indeed sets the flag for the VM to check AND writes to wakeup fd after that. The |
Is that file descriptor/socket documented to be used always and only for SIGINT? If so, great! Done. If not, I'm personally very wary of relying on undocumented behavior. |
@joelfrederico It's not fully documented when the descriptor is written to (I mean, at the same time the signal is caught by the process), but I think it's the only purpose of this function. I think I may ask about it in their repo, should I? Maybe they will document that more precisely, I can't imagine why they wouldn't want to. |
Hey it is documented! Python function Okay, so all you have to do is adapt your code to create a listening socket, pass that in to I feel silly for not noticing this earlier. Well done! |
And AFAICT this is cross-platform, so no futzing with ugly Windows hacks! I have no idea why PyZMQ gets this so wrong. That's just neat, I like it. |
So the issue seems to be that the low-level Python SIGINT handler doesn't have an analogue in Windows. You have to set it yourself the way PyZMQ does. So |
@joelfrederico, sorry, I wasn't precise: I said they didn't have it in 2014, but that's not true. The thing is that But now the approach works as expected, as I have tested -- the sigint test passes on Windows and on Linux. |
I'm going to close this as abandoned, but please feel free to reopen if anyone wants to pursue it... |
I'm not sure what you mean; this seems to have been updated to match the latest request. It needs a review now, but I tested it and it works. |
@vdrhtc I moved this to "Draft" to keep it off our radar until you were done with it. However, if you are done, feel free to move to "Ready for review". Thanks! |
Can you describe how to repro the crash you mention? |
@vdrhtc I'm going to do this rebase and take over this PR, sorry this has sat for so long. |
signal.set_wakeup_fd() used instead, start_event_loop() is interruptible now, allow_interrupt context manager, do not override None, SIG_DFL, SIG_IGN
I am now concerned why this did work for me locally before changing out the application for the loop in pause. My guess is something in detailed qt versions that I do not want to chase down. These tests also pass for me locally. |
They also pass on CI, so is all fine now? |
Yes I think so. |
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Oops, crossed with your review. |
my fault, shouldn't have decided to review after all :) do you want to address them separately? |
@anntzer @tacaswell The code to reproduce on Windows (in Jupyter): %matplotlib qt
from matplotlib import pyplot as plt
plt.plot([1,2])
plt.pause(.1)
plt.pause(.1)
plt.pause(.1) The problem seems to be that if
because nothing was actually written to the wakeup_fd . I don't understand how that can be.
Workaround fix: rsock.set_blocking(False)
@sn.activated.connect
def on_signal(*args):
try:
signal = rsock.recv(1) # clear the socket to re-arm the notifier
except BlockingIOError:
pass # false-trigger, nothing on rsock What should we do with this? Is there a better solution? |
PR Summary
This pull request fixes issue #13302 using
signal.set_wakeup_fd()
.The implemented
test_fig_sigint()
uses the example from the aforementioned issue. It's rather an integration test, though. It does not fail with the previous behaviour, but never passes either hanging the whole testing process.I also changed
test_fig_signals()
to pass with the new code.PR Checklist