Skip to content

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

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 24, 2023

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 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.)

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

@greglucas
Copy link
Contributor

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
Feel free to take any or all of that yourself.

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 plot(); show(); in memory_profiler and watch it build up) , and now that I'm seeing the retains in the WindowServerConnectionManager I'm wondering if some of that shared connection handling is keeping things alive and that would be worth looking at.

raise
print(stdout)
assert 'SUCCESS' in stdout
plt.figure()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QuLogic Do you remember why you added this plt.figure() line here (in #20907?) AFAICT it is not needed for the test and also locks in the running interactive framework for the main process, so I'm going to remove it(?)

Copy link
Member

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.

@anntzer
Copy link
Contributor Author

anntzer commented May 25, 2023

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.

@greglucas
Copy link
Contributor

Interestingly, I have had a similar experience debugging some previous issues with the reference counting where I added in some NSLog() statements, and that miraculously fixed the segfaults I was trying to locate. I would agree though, if it isn't quick to find something reliable then xfailing seems reasonable, or just removing the macosx from this list of backends because we aren't testing anything other than qt currently anyways. Another thing to try would be to call flush_events() to try and spin the event loop on the objc side.

@anntzer
Copy link
Contributor Author

anntzer commented May 26, 2023

Calling flush_events (or rather inlining its implementation into receivedData) doesn't seem to help, so I've just removed the test for now.
I've also integrated your patch now, thanks for doing it.


Edit: turns out I can just register a block (a lambda) instead and completely kill off WindowServerConnectionManager.

@anntzer anntzer force-pushed the mcc branch 2 times, most recently from 4a25169 to 084df64 Compare May 26, 2023 09:11
Copy link
Contributor

@greglucas greglucas left a 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?

@anntzer
Copy link
Contributor Author

anntzer commented May 27, 2023

Using mainQueue doesn't seem to help (but it may well be that it's indeed needed but not enough).
Perhaps we indeed need to repeat the wait but then I'm not sure why the printfs would sometimes help, and also I'd rather just defer implementing that to later (unless you see an easy way to do it).

@greglucas
Copy link
Contributor

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.

@jklymak
Copy link
Member

jklymak commented Jun 15, 2023

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...

@ksunden
Copy link
Member

ksunden commented Jun 15, 2023

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.

@ksunden ksunden closed this Jun 15, 2023
@ksunden ksunden reopened this Jun 15, 2023
@greglucas
Copy link
Contributor

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.

@greglucas
Copy link
Contributor

@anntzer, this needs a rebase now.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 20, 2023

Thanks for the bump, rebased.

@greglucas greglucas added this to the v3.8.0 milestone Jul 20, 2023
@ksunden
Copy link
Member

ksunden commented Jul 20, 2023

Failing test is one of the newly added tests

@greglucas
Copy link
Contributor

Failing test is one of the newly added tests

I can confirm this test fails locally for me as well. It is only the show + block=True combination that fails though which is odd. I think this combo can get skipped/xfailed for now with a comment indicating it is a known issue and if someone has time to investigate it then they can. But, this PR overall is a net gain and that can be saved for later IMO. My approval still stands.

@greglucas
Copy link
Contributor

I dug into this a bit more and it seems like the show() version isn't starting the event loop right away, but instead sitting idly by and waiting for something else to wake it up. If you run the test and don't touch the mouse it fails, but if you move the mouse over the canvas that is enough to wake it up and trigger the events and make the test pass. So, I think we need to somehow explicitly start an event loop when we go through show(). pause() explicitly calls canvas.start_event_loop(interval), which is why this test passes for pause().

@anntzer
Copy link
Contributor Author

anntzer commented Jul 30, 2023

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.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 7, 2023

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?

@anntzer anntzer force-pushed the mcc branch 2 times, most recently from 21d9589 to 3c5130a Compare August 7, 2023 22:01
anntzer and others added 2 commits August 8, 2023 00:02
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.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can't stop macosx mainloop SIGINT is ignored by MacOSX backend
6 participants