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

Clean up QTimer/QSocketNotifier when exiting Qt event loop #790

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stukowski
Copy link
Contributor

The QTimer and QSocketNotifier helper objects, created by the _notify_stream_qt() function as child objects of the global QApplication, need to be discarded after use when leaving the Qt event loop. Otherwise they keep piling up with each roundtrip between the Qt and the kernel event loop. This code change makes sure that these Qt objects destroy themselves at an appropriate time.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073
Copy link
Contributor

@ccordoba12, mind taking a look at this one as well?

@blink1073 blink1073 added the bug label Oct 22, 2021
@blink1073 blink1073 added this to the 6.4 milestone Oct 22, 2021
@ccordoba12
Copy link
Member

Thanks for the ping @blink1073!

@stukowski, what do you mean by this?

This code change makes sure that these Qt objects destroy themselves at an appropriate time.

I mean, what "an appropriate time" is?

@stukowski
Copy link
Contributor Author

I mean, what "an appropriate time" is?

I meant: Do not delete the QTimer and QSocketNotifier objects too early before they have fired, and do not call QObject.deleteLater() too late, because it requires a running Qt event loop.

Note that there are probably more ways of cleaning these objects up. I was just proposing one possible solution, which seems sufficiently simple.

Btw, I observed the problem of the timer and socket notifier not getting released by executing the second notebook cell below multiple times:

%gui qt
from IPython.external.qt_for_kernel import QtCore
for c in QtCore.QCoreApplication.instance().children(): print(c)

@blink1073 blink1073 modified the milestones: 6.4, 6.5 Oct 25, 2021
@ccordoba12
Copy link
Member

The QTimer and QSocketNotifier helper objects, created by the _notify_stream_qt() function as child objects of the global QApplication, need to be discarded after use when leaving the Qt event loop

This is the thing that worries me a bit. I mean, what does this mean for an interactive Matplotlib figure generated with the PyQt5 backend? Closing the figure or simply stop interacting with it after some time?

Pinging @tacaswell about this one too because he could be interested in knowing about it.

@stukowski
Copy link
Contributor Author

stukowski commented Oct 25, 2021

No, the QTimer and the QSocketNotifier objects are just an internal implementation detail of the _notify_stream_qt() function, and deleting these C++ "single-shot" objects after they have fired should have no effect on external Qt application codes (like Matplotlib).

Let me describe the problem again for which I am proposing a solution:
_notify_stream_qt() creates these two C++ objects to interrupt the Qt event loop as soon as there are new kernel events waiting to be processed. The problem is that _notify_stream_qt() creates a new set of these single-shot objects each time control returns to the Qt event loop (which happens many times during a session), but forgets to delete them after they have served their one-time purpose. Note that the two objects are created as child objects of the global Qt application. According to Qt's memory management model this means their lifetime is tied to the lifetime of the application object and these C++ objects don't get garbage collected. My pull requests fixes this by deleting the two helper objects explicitly (after they are no longer needed).

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 this pull request may close these issues.

3 participants