-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix support for Ctrl-C on the macosx backend. #25966
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
Nice, I think this could simplify a lot of the objc code, and I think this PR should probably clean up while you are working on this. See here for a quick hammer to your linked issue that seems to still work as expected for me. 5a30956 I also get the test failure timeout locally. If I click on the app icon to bring the window back into focus then the test passes. So, my guess is there is something with the event loop going to sleep and not being woken back up again? Also just as a note since I was looking at this portion of the code here. I am pretty sure we've got some leaks in the window objects on the objc side (loop over |
raise | ||
print(stdout) | ||
assert 'SUCCESS' in stdout | ||
plt.figure() |
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.
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'm not seeing any reason for it to be there either right now, and don't recall anything specific.
Thanks for the quick review. I just force-pushed a tiny cleanup for now as well as the change to the tests mentioned just above, I'll integrate your large cleanup patch later once we decide what to do with the problem below. I can repro the failure on test_other_signal_before_sigint, with the same observation as you. Doing a bit of printf-debugging (e.g. adding a printf at the beginning of receivedData, and also a print in the _WaitForStringPopen.wait_for loop) suggests to me that there may(?) be a race condition between set_wakeup_fd and PyErr_CheckSignals: it seems like receivedData sometimes gets called before PyErr_CheckSignals will actually call a signal handler. Somewhat curiously, adding the printf also makes that test pass sometimes, though not always, which supports the interpretation that there's a bit of delay needed before we can usefully call PyErr_CheckSignals (and the tiny delay from the printf helps making that call successful). Perhaps see python/cpython#74224 as well. If we can't figure that out quickly, I'd suggest just xfailing that test for now (perhaps on a shorter timeout...), as the behavior improvement on the plain test_sigint seems worth it by itself. |
Interestingly, I have had a similar experience debugging some previous issues with the reference counting where I added in some |
Calling flush_events (or rather inlining its implementation into receivedData) doesn't seem to help, so I've just removed the test for now. Edit: turns out I can just register a block (a lambda) instead and completely kill off WindowServerConnectionManager. |
4a25169
to
084df64
Compare
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.
This handles the KeyboardInterrupt as I'd expect now when testing manually on macosx and qt.
It would be nice to figure out what is going on with the multiple signals test case on macosx, but I don't think that should hold things up right now.
Some more comments for future selves:
https://docs.python.org/3/c-api/exceptions.html#c.PyErr_CheckSignals
If the function is called from a non-main thread, or under a non-main Python interpreter, it does nothing and returns 0.
Are we possibly calling this from a different background NSApp thread? since you aren't specifying which queue to put the notification on?
When I run pytest -n 4 lib/matplotlib/tests/test_backends_interactive.py::test_other_signal_before_sigint
and don't do anything. I get both tests to fail, [pause-kwargs1-macosx]
and [show-kwargs0-macosx]
. If I move my mouse over the figure (must activate NSTrackingRect?) during the test, the [show-kwargs0-macosx]
test succeeds, but the [pause-kwargs1-macosx]
fails, so there must be a difference between show and pause in the interaction here too.
Do we need to drain the filehandle/read what was there already and again call waitForDataInBackgroundAndNotify
?
Using mainQueue doesn't seem to help (but it may well be that it's indeed needed but not enough). |
No I don't see anything immediately obvious, my approval still stands, I was just throwing out ideas. It's just fine to leave that for later IMO. |
Is the test failure unrelated? I haven't dug into this, but would be happy to merge on Greg's recommendation. Except the MacOS test is failing... |
Closing and reopening to rerun tests with current main. I'm pretty sure the test failure is the tk problems on azure that we were having for a while (and actually only skips still as we await azure fixing things on their end). Though the run is old enough to not be accessible anymore, so can't be fully sure. |
Should be good to go from my perspective and CI is now passing, but note that there is some minor QT refactoring going on here as well. |
@anntzer, this needs a rebase now. |
Thanks for the bump, rebased. |
Failing test is one of the newly added tests |
I can confirm this test fails locally for me as well. It is only the |
I dug into this a bit more and it seems like the |
I'll have limited access to a dev machine for a few days, so feel free to push an xfail for now or a fix. |
Added the xfail for the show() + other signal case. At least from a quick look show(block=True) should call start_main_loop (via pyplot_show) so it's unclear why the event loop would not be started? |
21d9589
to
3c5130a
Compare
Support is largely copy-pasted from, and tests are shared with, the qt implementation (qt_compat._maybe_allow_interrupt), the main difference being that what we need from QSocketNotifier, as well as the equivalent for QApplication.quit(), are reimplemented in ObjC. qt_compat._maybe_allow_interrupt is also slightly cleaned up by moving out the "do-nothing" case (`old_sigint_handler in (None, SIG_IGN, SIG_DFL)`) and dedenting the rest, instead of keeping track of whether signals were actually manipulated via a `skip` variable. Factoring out the common parts of _maybe_allow_interrupt is left as a follow-up. (Test e.g. with `MPLBACKEND=macosx python -c "from pylab import *; plot(); show()"` followed by Ctrl-C.)
Support is largely copy-pasted from, and tests are shared with, the qt implementation (qt_compat._maybe_allow_interrupt, #13306), the main difference being that what we need from QSocketNotifier, as well as the equivalent for QApplication.quit(), are reimplemented in ObjC.
qt_compat._maybe_allow_interrupt is also slightly cleaned up by moving out the "do-nothing" case (
old_sigint_handler in (None, SIG_IGN, SIG_DFL)
) and dedenting the rest, instead of keeping track of whether signals were actually manipulated via askip
variable.Factoring out the common parts of _maybe_allow_interrupt is left as a follow-up.
(Test e.g. with
MPLBACKEND=macosx python -c "from pylab import *; plot(); show()"
followed by Ctrl-C.)Closes #3991 (which was closed as cantfix in #3991 (comment); likely much of #4006 can now be reverted).
Also closes #10002 by providing _macosx.stop as a private function to call [NSApp stop]; whether we want to make that public is another question.
PR summary
PR checklist