enthought / pyface

pyface: traits-capable windowing framework
Other
105 stars 55 forks source link

Hang / segfault at application close under PySide6 on macOS and Linux #1135

Closed mdickinson closed 2 years ago

mdickinson commented 2 years ago

The following script starts an application with a single window, and calls exit once the application is initialised.

It runs without error under PySide 2, but with PySide 6.3.1 on macOS it hangs: the GUI event loop is never exited. This appears to be the cause of errors in the current Envisage test suite with PySide 6 from pip.

Here's the script.

from pyface.api import GUI, ApplicationWindow
from traits.api import Event, HasTraits, Instance

class Application(HasTraits):

    gui = Instance(GUI, ())

    window = Instance(ApplicationWindow)

    application_initialized = Event

    def run(self):
        gui = self.gui

        self.window = ApplicationWindow()
        self.window.open()

        gui.set_trait_later(self, "application_initialized", self)
        gui.start_event_loop()

    def exit(self):
        self.window.destroy()
        self.window.closed = True

def main():
    app = Application()
    app.on_trait_change(lambda: app.exit(), "application_initialized")
    app.run()

if __name__ == "__main__":
    main()

In the hanging case, some debugging prints confirm that the exit method is being called as normal and that the window is being destroyed as expected, but the event loop does not exit.

The PySide 6 test environment was a Python 3.8 venv, created on macOS 12.4 / Intel with the following commands:

% python3.8 -m venv --clear ~/.venvs/testing && source ~/.venvs/testing/bin/activate
% python -m pip install --upgrade pip setuptools wheel
% python -m pip install pyface pyside6

Here's the full list of packages in the environment.

(testing) mdickinson@mirzakhani envisage % pip list
Package             Version
------------------- -------
importlib-resources 5.8.0
pip                 22.1.2
pyface              7.4.1
PySide6             6.3.1
PySide6-Addons      6.3.1
PySide6-Essentials  6.3.1
setuptools          62.6.0
shiboken6           6.3.1
traits              6.3.2
wheel               0.37.1
zipp                3.8.0
mdickinson commented 2 years ago

The same script hangs in the same way under Ubuntu Linux 20.04.1 running in a VirtualBox VM on my macOS host, again with PySide6 6.3.1 from PyPI.

The package list is identical to that above, except that there's also a bonus "pkg_resources 0.0.0" in the list. I don't think that's relevant here.

With PySide6 6.2.1 instead, the script segfaults. (Note that pip installing PySide6 6.2.1 does not bring in PySide6-Addons or PySide6-Essentials packages.)

corranwebster commented 2 years ago

Can confirm behaviour on MacOS Big Sur when as installed above. The issue also occurs when manually closing the window, so is not some weird traits/Qt interaction.

mdickinson commented 2 years ago

It's looking as though the issue may have to do with the WindowEventFilter. If I replace the __event_filter_default method in the (qt-specific) Window class with return None instead of return WindowEventFilter(self) then PySide 6 starts behaving as expected.

mdickinson commented 2 years ago

Here's the latest reproducer:

import weakref

from pyface.qt import QtCore, QtGui
from pyface.ui.qt4.gui import GUI
from pyface.ui.qt4.widget import Widget
from traits.api import HasTraits, Instance

class Window(Widget):
    """The toolkit specific implementation of a Window.  See the IWindow
    interface for the API documentation.
    """

    def open(self):
        # Create the control, if necessary.
        if self.control is None:
            self._create()

        self.show(True)
        self.opened = self

    def close(self):
        self.destroy()

    # Private interface ------------------------------------------------------

    def _create_control(self, parent):
        """Create a default QMainWindow."""
        control = QtGui.QMainWindow(parent)

        control.setEnabled(self.enabled)
        control.setVisible(self.visible)

        return control

    def destroy(self):
        if self.control is not None:
            control = self.control
            super().destroy()
            control.close()

    def __event_filter_default(self):
        return WindowEventFilter(self)

class WindowEventFilter(QtCore.QObject):
    """An internal class that watches for certain events on behalf of the
    Window instance.
    """

    def __init__(self, window):
        """Initialise the event filter."""
        QtCore.QObject.__init__(self)
        # use a weakref to fix finalization issues with circular references
        # we don't want to be the last thing holding a reference to the window
        self._window = weakref.ref(window)

    def eventFilter(self, obj, e):
        """Adds any event listeners required by the window."""

        window = self._window()

        # Sanity check.
        if window is None or obj is not window.control:
            return False

        typ = e.type()

        print(typ)

        if typ == QtCore.QEvent.Type.Close:
            # Do not destroy the window during its event handler.
            print("Destroying")

            GUI.invoke_later(window.close)

            if window.control is not None:
                e.ignore()

            return True

        if typ in {QtCore.QEvent.Type.Show, QtCore.QEvent.Type.Hide}:
            window.visible = window.control.isVisible()

        return False

class Application(HasTraits):

    window = Instance(Window)

    def run(self):
        app = QtGui.QApplication()
        self.window = Window()
        self.window.open()
        app.exec_()

def main():
    app = Application()
    app.run()

if __name__ == "__main__":
    main()
mdickinson commented 2 years ago

Sorry; copy-and-paste fail there. I've updated the message above with the actual script.

corranwebster commented 2 years ago

Yes, just tested with the basic

    from pyface.qt import QtCore, QtGui
    window = QtGui.QWindow()
    window.show()
    QtGui.QApplication.instance().exec_()

and could not reproduce.

Note that there are multiple different event filters applied in different places.

mdickinson commented 2 years ago

Another round of reductions. The following works under PySide 2 and hangs under PySide 6.3.1 on macOS; not yet tested on Linux.

import weakref

from pyface.qt import QtCore, QtGui
from pyface.ui.qt4.gui import GUI

class WindowEventFilter(QtCore.QObject):
    """An internal class that watches for certain events on behalf of the
    Window instance.
    """

    def __init__(self, window):
        """Initialise the event filter."""
        QtCore.QObject.__init__(self)
        # use a weakref to fix finalization issues with circular references
        # we don't want to be the last thing holding a reference to the window
        self._window = weakref.ref(window)

    def eventFilter(self, obj, e):
        """Adds any event listeners required by the window."""

        window = self._window()
        if e.type() == QtCore.QEvent.Type.Close:
            # Do not destroy the window during its event handler.
            GUI.invoke_later(window.close)

            if window._control is not None:
                e.ignore()

            return True

        return False

class Window:
    def __init__(self):
        self._control = None
        self._event_filter = None

    def _add_event_listeners(self):
        event_filter = WindowEventFilter(self)
        self._event_filter = event_filter
        self._control.installEventFilter(event_filter)

    def _remove_event_listeners(self):
        self._control.removeEventFilter(self._event_filter)
        self._event_filter = None

    def open(self):
        control = QtGui.QMainWindow()
        control.setEnabled(True)
        control.setVisible(True)
        self._control = control
        self._add_event_listeners()

    def close(self):
        control = self._control
        control.hide()
        control.deleteLater()
        self._remove_event_listeners()
        self._control = None
        control.close()

class Application:
    def __init__(self):
        self.window = None

    def run(self):
        app = QtGui.QApplication()
        self.window = Window()
        self.window.open()
        app.exec_()

def main():
    app = Application()
    app.run()

if __name__ == "__main__":
    main()
mdickinson commented 2 years ago

Under PySide 6, it looks as though the event filter shenanigans mean that the lastWindowClosed event on the QApplication is never emitted.

corranwebster commented 2 years ago

Note: this may be related https://github.com/enthought/traitsui/issues/1442

This is reported for PyQt5, however.

corranwebster commented 2 years ago

So all of the shenanigans is to allow code to listen for closing and veto the event if the app (or window, eg. for a document window that hasn't been saved) isn't ready to close.

I am wondering about the motivation of invoke_later. I can see that there may be limitations on what you can do in the middle of handling an event that will cause the application to quit (it's probably OK to throw up a modal dialog saying "do you want to close?" or even a series of modals, but more sophisticated things may be harder)

mdickinson commented 2 years ago

What's confusing me is why the last posted code isn't working under PySide 6.

But for some reason we're just not getting that lastWindowClosed event.

mdickinson commented 2 years ago

Here's an unexpected observation: if I comment out the control.hide() statement in the Window.close method, that fixes the hang.

corranwebster commented 2 years ago
  • That last control.close() in the Window.close method should be emitting a close event. From the docs:

    Close events [...] are also sent when you call QWidget::close() to close a widget programmatically.

As best I can tell, it is being sent. If you don't delete the event filter then you can see the second event being handled by it.

Here's an unexpected observation: if I comment out the control.hide() statement in the Window.close method, that fixes the hang.

That makes a certain sense, I can see the thought that if a window is hidden then it shouldn't be involved in the lastWindowClosed computations.

corranwebster commented 2 years ago

So I'm in two minds about the "right" fix:

Both have the potential for weird side-effects/finalization issues.

Edit: third possibility is:

mdickinson commented 2 years ago

As best I can tell, it is being sent. If you don't delete the event filter then you can see the second event being handled by it.

Hmm. I tried that, and that's not what I'm seeing. The second close event isn't happening for me. (Note that there are two close events sent at the original close point.)

Here's a hacked up script demonstrating that:

import time
import weakref

from pyface.qt import QtCore, QtGui
from pyface.ui.qt4.gui import GUI

class WindowEventFilter(QtCore.QObject):
    """An internal class that watches for certain events on behalf of the
    Window instance.
    """

    def __init__(self, window):
        """Initialise the event filter."""
        QtCore.QObject.__init__(self)
        # use a weakref to fix finalization issues with circular references
        # we don't want to be the last thing holding a reference to the window
        self._window = weakref.ref(window)

    def eventFilter(self, obj, e):
        """Adds any event listeners required by the window."""

        print(f"{time.perf_counter():.2f} Got event {e} from {obj}")

        window = self._window()
        if e.type() == QtCore.QEvent.Type.Close:
            # Do not destroy the window during its event handler.
            GUI.invoke_after(2000, window.close)

            if window._control is not None:
                e.ignore()

            return True

        return False

class Window:
    def __init__(self):
        self._control = None
        self._event_filter = None

    def _add_event_listeners(self):
        event_filter = WindowEventFilter(self)
        self._event_filter = event_filter
        self._control.installEventFilter(event_filter)

    def _remove_event_listeners(self):
        # self._control.removeEventFilter(self._event_filter)
        self._event_filter = None

    def open(self):
        control = QtGui.QMainWindow()
        control.setEnabled(True)
        control.setVisible(True)
        self._control = control
        self._add_event_listeners()

    def close(self):
        control = self._control
        control.hide()
        control.deleteLater()
        self._remove_event_listeners()
        self._control = None
        control.close()

class Application:
    def __init__(self):
        self.window = None

    def run(self):
        app = QtGui.QApplication()
        self.window = Window()
        self.window.open()
        app.exec_()

def main():
    app = Application()
    app.run()

if __name__ == "__main__":
    main()
corranwebster commented 2 years ago

From the Qt docs on lastWindowClosed:

This signal is emitted from exec() when the last visible primary window (i.e. top level window with no transient parent) is closed.

mdickinson commented 2 years ago

I think this is no longer useful, but I'll post it since I have it: here's a Pyface-free version of the original code that exhibits the same behaviour:

try:
    from PySide6 import QtCore
    from PySide6.QtWidgets import QApplication, QMainWindow
except ImportError:
    from PySide2 import QtCore
    from PySide2.QtWidgets import QApplication, QMainWindow

class WindowEventFilter(QtCore.QObject):

    close_later = QtCore.Signal()

    def __init__(self, window):
        QtCore.QObject.__init__(self)
        self._window = window

    def eventFilter(self, obj, e):
        window = self._window
        if e.type() != QtCore.QEvent.Type.Close:
            return False

        self.close_later.emit()
        if window._control is not None:
            e.ignore()
        return True

class Window:
    def __init__(self):
        self._control = None
        self._event_filter = None

    def _add_event_listeners(self):
        event_filter = WindowEventFilter(self)
        self._event_filter = event_filter
        self._control.installEventFilter(event_filter)
        self._event_filter.close_later.connect(
            self.close, QtCore.Qt.QueuedConnection)

    def _remove_event_listeners(self):
        self._control.removeEventFilter(self._event_filter)
        self._event_filter = None

    def open(self):
        control = QMainWindow()
        control.setEnabled(True)
        control.setVisible(True)
        self._control = control
        self._add_event_listeners()

    def close(self):
        self._remove_event_listeners()
        control = self._control
        control.hide()
        control.deleteLater()
        self._control = None
        control.close()

def main():
    app = QApplication()
    window = Window()
    window.open()
    app.exec_()

if __name__ == "__main__":
    main()
corranwebster commented 2 years ago
    def _remove_event_listeners(self):
        # self._control.removeEventFilter(self._event_filter)
        self._event_filter = None

You are still removing the reference to self._event_filter so it may be gc'd?

mdickinson commented 2 years ago

You are still removing the reference to self._event_filter so it may be gc'd?

Ah, good point; that could be it.

mdickinson commented 2 years ago

Here's a backtrace from the segfault on Linux.

(gdb) bt
#0  0x00007f92d459e9a2 in Shiboken::BindingManager::retrieveWrapper(void const*) ()
   from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/shiboken6/libshiboken6.abi3.so.6.3
#1  0x00007f92d4c15185 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/libpyside6.abi3.so.6.3
#2  0x00007f92d4c1585a in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/libpyside6.abi3.so.6.3
#3  0x00007f92d387c55e in QVariant::~QVariant() () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#4  0x00007f92d385740c in QObjectPrivate::~QObjectPrivate() ()
   from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#5  0x00007f92d391c379 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#6  0x00007f92d391c033 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#7  0x00007f92d391c187 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#8  0x00007f92d391c3b9 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#9  0x00007f92d38572e6 in QObjectPrivate::~QObjectPrivate() ()
   from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#10 0x00007f92d38655ac in QObject::~QObject() () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#11 0x00007f92b9acdf37 in ?? ()
   from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/plugins/xcbglintegrations/libqxcb-glx-integration.so
#12 0x00007f92d3aa6032 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#13 0x00007f92d3aa86d3 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#14 0x00007f92d3aa6299 in ?? () from /home/mdickinson/.venvs/envisage-pyside6/lib/python3.8/site-packages/PySide6/Qt/lib/libQt6Core.so.6
#15 0x00007f92d56d58a7 in __run_exit_handlers (status=0, listp=0x7f92d587b718 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, 
    run_dtors=run_dtors@entry=true) at exit.c:108
#16 0x00007f92d56d5a60 in __GI_exit (status=<optimised out>) at exit.c:139
#17 0x00007f92d56b308a in __libc_start_main (main=0x4eead0 <main>, argc=2, argv=0x7ffd0242e048, init=<optimised out>, fini=<optimised out>, 
    rtld_fini=<optimised out>, stack_end=0x7ffd0242e038) at ../csu/libc-start.c:342
#18 0x00000000005fa5ce in _start ()
mdickinson commented 2 years ago

I'll open a separate issue for the segfault.