Leaking top level widgets

[tlandschoff-scale]

Hi *,

our pipeline broke lately, running in to segfaults. After a while of searching, it appears that some widgets live on until the the test session concludes. Somehow, during interpreter shutdown, this leads into updating dead QAbstractItemModel instances, which causes the crash.

It is still unclear to me how this unfolds, but the basics are reproducible in pytest-qt at 3e593f2. Look at this (new) test case (tests/test_widget_leak.py):

import pytest

from pytestqt.qt_compat import qt_api


@pytest.mark.parametrize("n", range(42))
def test_widget_leak(qapp, n):
    widget = qt_api.QtWidgets.QWidget()
    widget.show()
    qapp.processEvents()
    assert len(qapp.topLevelWidgets()) == 1

I get a successful run exercising only this new test case:

$ tox -e py38-pyqt5 -- tests/test_widget_leak.py 
...
========== 42 passed in 0.12s ==========

  py38-pyqt5: OK (5.39=setup[5.00]+cmd[0.40] seconds)
  congratulations :) (5.52 seconds)

But running some existing tests cases first, this fails because there are additional top level widgets. Also, the newly created widgets are kept now:

$ tox -e py38-pyqt5 -- tests/test_basics.py::test_stop tests/test_widget_leak.py 
...
========== short test summary info ==========
FAILED tests/test_widget_leak.py::test_widget_leak[0] - assert 2 == 1
FAILED tests/test_widget_leak.py::test_widget_leak[1] - assert 3 == 1
FAILED tests/test_widget_leak.py::test_widget_leak[2] - assert 4 == 1
...
========== 42 failed, 1 passed in 0.28s ==========

I failed to directly and explicitly delete/destroy those top level widgets (there is no usable delete/destroy method exposed in Python). The following work around seems to get rid of extra top levels:

diff --git a/src/pytestqt/plugin.py b/src/pytestqt/plugin.py
index fab2c72..fca66b7 100644
--- a/src/pytestqt/plugin.py
+++ b/src/pytestqt/plugin.py
@@ -50,9 +50,15 @@ def qapp_cls():
     """
     return qt_api.QtWidgets.QApplication
 
+@pytest.fixture
+def qapp(qapp_session):
+    try:
+        yield qapp_session
+    finally:
+        _delete_toplevels(qapp_session)
 
 @pytest.fixture(scope="session")
-def qapp(qapp_args, qapp_cls, pytestconfig):
+def qapp_session(qapp_args, qapp_cls, pytestconfig):
     """
     Fixture that instantiates the QApplication instance that will be used by
     the tests.
@@ -76,6 +82,25 @@ def qapp(qapp_args, qapp_cls, pytestconfig):
         return app
 
 
+def _delete_toplevels(app):
+    QTimer = qt_api.QtCore.QTimer
+
+    def delete_core():
+        top_levels = app.topLevelWidgets()
+        if top_levels:
+            top = top_levels.pop()
+            if top:
+                top.deleteLater()
+            QTimer.singleShot(0, delete_core)
+        else:
+            loop.exit(0)
+
+    QTimer.singleShot(0, delete_core)
+    loop = qt_api.QtCore.QEventLoop()
+    loop.exec_()
+    assert not app.topLevelWidgets()
+
+
 # holds a global QApplication instance created in the qapp fixture; keeping
 # this reference alive avoids it being garbage collected too early
 _qapp_instance = None

I have no idea though if this has any bad side effects. Also, I am not sure what causes the survival of the widgets. Any insights appreciated.

Greetings, Torsten

[nicoddemus]

Hi @tlandschoff-scale,

Indeed it is the user's responsibility to close the widgets, that's why there's the qtbot.addWidget method.

I failed to directly and explicitly delete/destroy those top level widgets (there is no usable delete/destroy method exposed in Python).

Just closing (via .close()) the top level widgets is enough to effectively delete them in my experience.

We do mention to use addWidget in the tutorial:

Registering widgets is not required, but recommended because it will ensure those widgets get properly closed after each test is done.

I also have seen crashes when one does not close/destroy things properly, which is unfortunate.

Hope this helps.

[tlandschoff-scale]

Hi Bruno,

Indeed it is the user's responsibility to close the widgets

1. this does not appear to be particularly robust (one broken test can go unnoticed for a long time that way)
2. without qtbot (and qtbot.addWidget) I did not see these problems

Especially, my new test did only fail when adding e.g. this existing test from pytest-qt to the test run:

def test_stop(qtbot, timer):
    widget = qt_api.QtWidgets.QWidget()
    qtbot.addWidget(widget)

    with qtbot.waitExposed(widget):
        widget.show()

    timer.single_shot_callback(widget.close, 0)
    qtbot.stop()

Given that adding this test to the run makes the fault appear, what does this code to wrong then?

Just closing (via .close()) the top level widgets is enough to effectively delete them in my experience.

I tried (on our code base), they still stay alive when using qtbot.

I also have seen crashes when one does not close/destroy things properly, which is unfortunate.

Not the fault of pytest-qt, I consider PyQt and PySide quite fragile in that matter. You get the C++ behaviour "API misuse => crash" from Python that way, without of the tool support you got in C++. But I think that is out of scope here, I just would like better isolation between tests using pytest-qt.

Greetings, Torsten

[tlandschoff-scale]

Out of curiosity, I ran the tests with this patch:

diff --git a/tests/conftest.py b/tests/conftest.py
index 6010c31..f3cec82 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -7,6 +7,13 @@ from pytestqt.qt_compat import qt_api
 pytest_plugins = "pytester"
 
 
+def pytest_runtest_logfinish(nodeid, location):
+    app = qt_api.QtWidgets.QApplication.instance()
+    if app is not None:
+        n = len(app.topLevelWidgets())
+        print(f"{nodeid}: {n} widgets in the balance.")
+
+
 @pytest.fixture
 def stop_watch():
     """

Turns out that pytest-qt's own tests keep up to 19 widgets lying around:

.tests/test_wait_signal.py::test_blockers_handle_exceptions[False-multiple]: 19 widgets in the balance.
.tests/test_wait_signal.py::test_blockers_handle_exceptions[False-callback]: 19 widgets in the balance.
.tests/test_wait_signal.py::test_wait_twice[True-True]: 0 widgets in the balance.

Probably, test_wait_twice actually deletes those because an actual event loop is run there.

Seems like pytest-qt uses deleteLater to clean up widgets after running a test case:

def _close_widgets(item):
    """
    Close all widgets registered in the pytest item.
    """
    widgets = getattr(item, "qt_widgets", None)
    if widgets:
        for w, before_close_func in item.qt_widgets:
            w = w()
            if w is not None:
                if before_close_func is not None:
                    before_close_func(w)
                w.close()
                w.deleteLater()
        del item.qt_widgets

Note the docs on deleteLater:

If the event loop is not running when this function is called (e.g. deleteLater() is called on an object before [QCoreApplication::exec](https://doc.qt.io/qt-5/qcoreapplication.html#exec)()), the object will be deleted once the event loop is started.

So maybe, the simple fix would be to bring up a new QEventLoop in that code to actually purge those deleted widgets.

Greetings, Torsten

[nicoddemus]

Hi Torsten,

Unfortunately changing pytest-qt to close all widgets implicitly now (using app.topLevelWidgets()) can have unintended consequences. I know because we did an experiment in our large code base, and we started to get crashes left and right. Do not get me wrong, I'm sure the problem is somewhere in our code, but my point is that changing this on pytest-qt is dangerous and will probably cause suites to fail/crash.

Note that you do not need to patch pytest-qt to get that behavior, you can instead implement it using an autouse fixture in your conftest.py file:

@pytest.fixture
def close_all_widgets():
    yield
    app = QApplication.instance()
    if app is not None:
         for w in app.topLevelWidgets():
             w.close()

But as I said, I'm hesitant to add support for this directly in pytest-qt. @The-Compiler any thoughts?

[The-Compiler]

Sorry for the radio silence, this got lost with all the Europython and pytest training stuff!

I feel like we should still aim to:

• Find out at least why things are segfaulting when we do this
• Make sure our own testsuite doesn't leak any widgets after tests (now it does apparently, and it's unclear to me why!)
• Perhaps even offer something that fails tests when any widgets are leaked after them?

So yeah. This won't be a priority for myself for the forseeable future I'm afraid, but I still think we should investigate some more at some point.