Skip to content

Fix last QMenu leak and its associated segfaults#2522

Merged
j9ac9k merged 9 commits intopyqtgraph:masterfrom
pijyoi:fix-qmenu-leak-without-segfault
Nov 26, 2022
Merged

Fix last QMenu leak and its associated segfaults#2522
j9ac9k merged 9 commits intopyqtgraph:masterfrom
pijyoi:fix-qmenu-leak-without-segfault

Conversation

@pijyoi
Copy link
Copy Markdown
Contributor

@pijyoi pijyoi commented Nov 5, 2022

Fixes #2497.
Completes #2518.
With the segfaults being attributed to the reason listed in #2520, this PR avoids taking the gui thread object.
This includes:

  1. switching SignalProxy to use QTimer instead of ThreadsafeTimer
  2. skipping tests on widgets that take the gui thread object in their implementation

Thus there are 2 bugs in PySide binding

  1. QMenu leak on QMenu::addMenu(QMenu*) (as detailed in partial fix for Qmenu leak #2518)
  2. gui thread object segfault since PySide6 >= 6.3 (as detailed in test_busycursor.py segfaults on Linux PySide6 6.3, 6.4 #2520)

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Nov 5, 2022

With the cause of the segfault out of the way, a cleaner implementation of building the mouse submenu can be done.

@pijyoi pijyoi force-pushed the fix-qmenu-leak-without-segfault branch 2 times, most recently from 4ca79f7 to 7875269 Compare November 7, 2022 14:24
@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Nov 8, 2022

I am trying to move away from the use of ThreadsafeTimer in SignalProxy, while maintaining backwards compatibility.
Possibilities:

  1. create another class (e.g. SignalThrottler) that uses QTimer and change all library uses to this new class.
    • adding yet another public class
  2. add a parameter threadSafe=True to SignalProxy and change all library uses to adding threadSafe=False. (this is what is currently implemented in this PR)
    • unwieldy but putting the burden on library developers doesn't seem too onerous
  3. create a new class SignalProxyThreadsafe and bind it to pg.SignalProxy. Change SignalProxy to use QTimer. Thus all library uses will end up using the QTimer version, while external users that use pg.SignalProxy still use the ThreadsafeTimer version.
    • having behavior change depending on how you import it doesn't sound good
  4. ignore backwards compatibility and just switch to QTimer.

@pijyoi pijyoi force-pushed the fix-qmenu-leak-without-segfault branch from 7875269 to 1d3ac9a Compare November 8, 2022 13:45
@pijyoi pijyoi marked this pull request as ready for review November 8, 2022 13:58
@pijyoi pijyoi force-pushed the fix-qmenu-leak-without-segfault branch from f13bb82 to 6760d67 Compare November 12, 2022 00:53
the test is actually bogus. the current implementation is such that it
has no C++ children, thus qObjectTree() will return only one element.
Yet if the menu did actually have any children, the test would fail on
PyQt bindings; because the Python wrappers of the children are still
alive (but the wrapped C++ objects are deleted)
there is already a config option "mouseRateLimit" for such a purpose,
thus using SignalProxy here would be a bad example.
@pijyoi pijyoi force-pushed the fix-qmenu-leak-without-segfault branch from 6760d67 to c64a5e3 Compare November 19, 2022 04:36
@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Nov 19, 2022

With the change of ubuntu-latest to ubuntu-22.04, the Linux / Python 3.9 / PySide6.2 pipeline segfaults.
Testing on py38, py39, py310 with PySide6.2 showed segfaults on py38, py39 (but not py310).
UPDATE: pyside6.1 and pyside6.0 also has segfaults.

Fixed by skipping test_busycursor and test_progressdialog also on older PySide6.

@pijyoi pijyoi force-pushed the fix-qmenu-leak-without-segfault branch from f192973 to d9142ec Compare November 19, 2022 12:13
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Nov 26, 2022

Thanks for your work on this @pijyoi I know this one was a doozy to sort out.

@j9ac9k j9ac9k merged commit dc40d07 into pyqtgraph:master Nov 26, 2022
@pijyoi pijyoi deleted the fix-qmenu-leak-without-segfault branch November 26, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

QMenus not destroyed (memory leak)

2 participants