Clean up Qt socket notifier to avoid spurious interrupt handler calls - #30209
Clean up Qt socket notifier to avoid spurious interrupt handler calls#30209anntzer merged 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
|
Though I haven't seen official mention of the need to call .deleteLater(), I certainly have seen before that Python reference counts reaching zero isn't enough to clean up all state associated with QObjects in PyQt. Here's are some mentions of this on the web: It does seem, from running gc.get_referrers(), that there aren't any other Python references to this QSocketNotifier, so it does seem to be living on exclusively in the C++ layer unless explicitly deleted. |
Sorry, something went wrong.
|
Oh I see. _allow_interrupt() is backend-agnostic, so Qt things don't belong there, hence the tests failing on macos. This will have to be done differently. |
Sorry, something went wrong.
|
Changed to put the cleanup in the backend-specific code by connecting the event loop's aboutToQuit signal to the socket notifier's deleteLater() method edit: (that doesn't work for QEventLoop) by explicitly calling sn.deleteLater() from handle_sigint(). I've tested with PyQt6, PySide6, PyQt5, and PySide2. Looks like this was only ever an issue in PyQt: before after PyQt6 bad good PySide6 good good PyQt5 bad good PySide2 good good The PySide2 testing was not done in a proper dev environment, but by just editing the same file in a matplotlib 3.10.3 installation installed with pip in a Python3.10 virtual environment - seems like the setup process for a proper dev environment requires at least Python 3.11, but the last Python version supported by PySide2 was 3.10. Testing for the other bindings was done in a dev environment on Linux with Python 3.13.3. |
Sorry, something went wrong.
Closes matplotlib#29688 Objects without a parent are not necessarily cleaned up in `PyQt5/6` when their reference count reaches zero, and must be explicitly cleaned up with `deleteLater()` This prevents the notifier firing after the signal handling was supposed to have been reset to its previous state. Rather than have both `bakend_bases._allow_interrupt()` and `backend_qt._allow_interrupt_qt()` hold a reference to the notifier, we pass it to the backend-specific `handle_signint()` function for cleanup. Note the approach to cleaning up the notifier with `.deleteLater()` followed by `sendPostedEvents()` is the documented workaround for when immediate deletion is desired: https://doc.qt.io/qt-6/qobject.html#deleteLater This ensures the object is still deleted up even if the same event loop does not run again.
|
Thanks for the patch! |
Sorry, something went wrong.
…urious interrupt handler calls
…209-on-v3.10.x Backport PR #30209 on branch v3.10.x (Clean up Qt socket notifier to avoid spurious interrupt handler calls)
Closes #29688
Objects without a parent are not necessarily cleaned up in PyQt when their reference count reaches zero, and must be explicitly cleaned up with deleteLater()
This prevents the notifier firing after the signal handling was supposed to have been cleaned up, which can cause spurious errors to be printed if it runs upon further interrupts or at interpreter shutdown.
I've tested this on Linux with Python 3.13, using both of
Previously, running the following with python -i script.py:
and then pressing ctrl-C in the terminal whilst the figure was shown, would cause a traceback and error message OSError: [Errno 9] Bad file descriptor to be printed as the interpreter exited (by e.g. subsequently pressing ctrl-D), and at least for me, cause the controlling terminal to exit for some reason.
After this change interpreter shutdown appears clean.
Similarly, as in the original bug report, running ipython and then entering in the REPL:
and pressing ctrl-C would result in an infinite loop of OSError: [Errno 9] Bad file descriptor error messages, and with this change results in a single KeyBoardInterrupt traceback as expected.
PR checklist
- [] Plotting related features are demonstrated in an example N/A
- [ ] New Features and API Changes are noted with a directive and release note N/A
- [] Documentation complies with general and docstring guidelines N/A
I've checked the docs build and the tests run - one test fails: test_font_heuristica[pdf] but it fails regardless of this change. I have only tested on Linux with Python 3.13.3.