"RuntimeError: Failed to disconnect" with `PySide6==6.7.0`

bersbersbers commented 7 months ago

I recently tried upgrading to an early PySide6==6.7.0 (via https://download.qt.io/snapshots/ci/pyside/6.7/latest/pyside6), and I find that pytest then issues a lot of warnings that I cannot seem to suppress:

c:\Code\project.venv\Lib\site-packages\pytestqt\wait_signal.py:741: RuntimeError: Failed to disconnect (<bound method _AbstractSignalBlocker._quit_loop_by_timeout of <pytestqt.wait_signal.SignalBlocker object at 0x000001BE3BDA6390>>) from signal "timeout()". signal.disconnect(slot)

filterwarnings = ["ignore"] does not seem to have an effect, possibly because RuntimeError is not a proper warning.

nicoddemus commented 7 months ago

It is probably because the exception is being raised inside the Qt event loop.

penguinpee commented 6 months ago

I'm seeing the same here:

=================================== FAILURES ===================================
________________________________ test_destroyed ________________________________
CALL ERROR: Exceptions caught in Qt event loop:
________________________________________________________________________________
RuntimeError: Internal C++ object (Obj) already deleted.
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "/builddir/build/BUILDROOT/python-pytest-qt-4.4.0-2.fc41.x86_64/usr/lib/python3.12/site-packages/pytestqt/wait_signal.py", line 219, in _quit_loop_by_signal
    self._cleanup()
  File "/builddir/build/BUILDROOT/python-pytest-qt-4.4.0-2.fc41.x86_64/usr/lib/python3.12/site-packages/pytestqt/wait_signal.py", line 226, in _cleanup
    _silent_disconnect(signal, self._quit_loop_by_signal)
  File "/builddir/build/BUILDROOT/python-pytest-qt-4.4.0-2.fc41.x86_64/usr/lib/python3.12/site-packages/pytestqt/wait_signal.py", line 741, in _silent_disconnect
    signal.disconnect(slot)
SystemError: <class 'RuntimeError'> returned a result with an exception set
________________________________________________________________________________
----------------------------- Captured stderr call -----------------------------
Exceptions caught in Qt event loop:
________________________________________________________________________________
RuntimeError: Internal C++ object (Obj) already deleted.
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "/builddir/build/BUILDROOT/python-pytest-qt-4.4.0-2.fc41.x86_64/usr/lib/python3.12/site-packages/pytestqt/wait_signal.py", line 219, in _quit_loop_by_signal
    self._cleanup()
  File "/builddir/build/BUILDROOT/python-pytest-qt-4.4.0-2.fc41.x86_64/usr/lib/python3.12/site-packages/pytestqt/wait_signal.py", line 226, in _cleanup
    _silent_disconnect(signal, self._quit_loop_by_signal)
  File "/builddir/build/BUILDROOT/python-pytest-qt-4.4.0-2.fc41.x86_64/usr/lib/python3.12/site-packages/pytestqt/wait_signal.py", line 741, in _silent_disconnect
    signal.disconnect(slot)
SystemError: <class 'RuntimeError'> returned a result with an exception set
________________________________________________________________________________

That test succeeded with PySide 6.6.2.

nicoddemus commented 6 months ago

A SystemError usually means a problem with the C code (in this case the C++ bindings).

Perhaps it would be good create a MWE and post this to PySide's tracker.

bersbersbers commented 6 months ago

Well, one MWE (without pytest-qt) is this:

from PySide6.QtCore import Signal
from PySide6.QtWidgets import QApplication, QMainWindow

class Window(QMainWindow):
    signal = Signal()

    def __init__(self):
        super().__init__()
        self.signal.connect(self.Slot)
        self.signal.disconnect(self.Slot)
        self.signal.disconnect(self.Slot)

    def Slot(self): ...

app = QApplication()
window = Window()
print("Done.")

But I somehow feel reporting this to PySide will give us the question "why are you disconnecting twice"? :)

I think the comment in line 738 is relevant here - is that what you have in mind?

https://github.com/pytest-dev/pytest-qt/blob/aac9be7936dcab86be23788c3b081fde078b4dc0/src/pytestqt/wait_signal.py#L736-L743

I don't suppose you remember which code example made you do https://github.com/pytest-dev/pytest-qt/commit/86e41e3f5a09213d338b916e507a9d70bb0619a8 :)

bersbersbers commented 6 months ago

Okay, here's a real one:

from PySide6.QtWidgets import QWidget

def test_disconnect(qtbot):
    widget = QWidget()
    qtbot.addWidget(widget)
    _ = qtbot.waitSignal(widget.windowTitleChanged, timeout=10000)
    widget.windowTitleChanged.emit("")

> pytest bug.py

======================================================================================= test session starts ======================================================================================== 
platform win32 -- Python 3.12.3, pytest-8.1.1, pluggy-1.5.0
PySide6 6.7.0 -- Qt runtime 6.7.0 -- Qt compiled 6.7.0
rootdir: C:\Code\project
plugins: qt-4.4.0
collected 1 item                                                                                                                                                                                     

bug.py .                                                                                                                                                                                      [100%] 

========================================================================================= warnings summary ========================================================================================= 
bug.py::test_disconnect
  c:\Code\project\.venv\Lib\site-packages\pytestqt\wait_signal.py:741: RuntimeError: Failed to disconnect (<bound method _AbstractSignalBlocker._quit_loop_by_timeout of <pytestqt.wait_signal.SignalBlocker object at 0x00000265849F0B60>>) from signal "timeout()".
    signal.disconnect(slot)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=================================================================================== 1 passed, 1 warning in 0.10s ===================================================================================

bersbersbers commented 6 months ago

And a workaround:

from PySide6.QtWidgets import QWidget

def test_disconnect(qtbot):
    widget = QWidget()
    qtbot.addWidget(widget)
    _ = qtbot.waitSignal(widget.windowTitleChanged, timeout=1)
    del _
    widget.windowTitleChanged.emit("")

Edit: yes, that fixes the issue in my big app.

Edit: another workaround is just to not assign to _ in the first place.

@nicoddemus, do you think this is reportable to PySide in this form, or will they want to see something without pytest-qt? Signal disconnect behaviors have been subject of bug fixing for the last 10 or so releases...

nicoddemus commented 6 months ago

@bersbersbers sorry for the delay.

The problem is:

pytest-qt does the right thing and ignores the runtime error in case it is disconnected twice. In our case it is OK because the signal might have been disconnected by the user at that point.
PySide is emitting a warning that somehow is bypassing the standard warnings filter, that's why using filterwarnings in pytest has no effect.

Here is a MWE which shows the problem, without using pytest-qt:

import warnings
from PySide6.QtCore import Signal
from PySide6.QtWidgets import QApplication, QMainWindow

warnings.filterwarnings('ignore')

class Window(QMainWindow):
    signal = Signal()

    def __init__(self):
        super().__init__()
        self.signal.connect(self.Slot)
        self.signal.disconnect(self.Slot)
        self.signal.disconnect(self.Slot)

    def Slot(self): ...

warnings.warn(Warning("some warning"))
app = QApplication()
window = Window()
print("Done")

While we should see no warnings at all due to warnings.filterwarnings, this prints:

e:\projects\pytest-qt\tests\foo.py:15: RuntimeError: Failed to disconnect (<bound method Window.Slot of <__main__.Window(0x2220ac06c40) at 0x000002220CF995C0>>) from signal "signal()".
  self.signal.disconnect(self.Slot)
Done

As a sanity check, if we comment out warnings.filterwarnings, we see both warnings as expected:

e:\projects\pytest-qt\tests\foo.py:19: Warning: some warning
  warnings.warn(Warning("some warning"))
e:\projects\pytest-qt\tests\foo.py:15: RuntimeError: Failed to disconnect (<bound method Window.Slot of <__main__.Window(0x28947215dd0) at 0x0000028949219680>>) from signal "signal()".
  self.signal.disconnect(self.Slot)
Done

bersbersbers commented 6 months ago

@nicoddemus thanks - and no worries about the delay. I have found a workaround for my issue, and will work on getting this solved only as a bonus for others, so I have no strict timeline here. In summary, I see two issues:

https://github.com/pytest-dev/pytest-qt/issues/552#issuecomment-2081483901 shows an non-pytest-qt MWE that generates a non-suppressable warning when disconnecting a signal twice.
https://github.com/pytest-dev/pytest-qt/issues/552#issuecomment-2078013599 shows a (still pytest-qt-based) MWE that shows how PySide behaves differently depending on whether qtbot.waitSignal is assigned an output variable or not.

I will report both issues to PySide and updates this thread here.

nicoddemus commented 6 months ago

Thanks!

I will close for now, but looking forward to the follow ups. 👍

bersbersbers commented 6 months ago

Follow https://bugreports.qt.io/projects/PYSIDE/issues/PYSIDE-2705 if interested.

nicoddemus commented 6 months ago

Thanks! Watching that issue. :+1:

bersbersbers commented 6 months ago

(Hopefully) fixed with PySide6 6.7.1 and 6.8.0.

bersbersbers commented 6 months ago

I tested this on an early 6.7.1 (pip install https://download.qt.io/snapshots/ci/pyside/6.7/latest/pyside6/shiboken6-6.7.0a1.dev1714496642-cp39-abi3-win_amd64.whl https://download.qt.io/snapshots/ci/pyside/6.7/latest/pyside6/PySide6_Essentials-6.7.0a1.dev1714496642-cp39-abi3-win_amd64.whl) and it is working fine. We are still getting the warning, but it can be suppressed now. Maybe you want to extend your try ... except block to ignore just that particular warning.

nicoddemus commented 6 months ago

yeah, using catch_warnings around that code would do the trick. Would you like to open a PR with that?

bersbersbers commented 6 months ago

I might - however, I came across this note:

The catch_warnings manager works by replacing and then later restoring the module’s showwarning() function and internal list of filter specifications. This means the context manager is modifying global state and therefore is not thread-safe.

This means it might impact the user's own warnings filters, which is not ideal.

nicoddemus commented 6 months ago

This means it might impact the user's own warnings filters, which is not ideal.

But this is temporary, it will not be a problem because the context manager will be short-lived:

 def _silent_disconnect(signal, slot): 
     """Disconnects a signal from a slot, ignoring errors. Sometimes 
     Qt might disconnect a signal automatically for unknown reasons. 
     """ 
     with warnings.catch_warnings():
         # PySide 6.7+ issues a UserWarning instead of an exception.
         warnings.filterwarnings("ignore", category=UserWarning)
         try: 
             signal.disconnect(slot) 
         except (TypeError, RuntimeError):  # pragma: no cover 
             pass

bersbersbers commented 6 months ago

Well, imagine this situation:

Thread 1:

with warnings.catch_warnings():
    warnings.filterwarnings("ignore", category=UserWarning)     
    thread_1_work()

Thread 2:

with warnings.catch_warnings():
    warnings.filterwarnings("ignore", category=OtherWarning)     
    thread_2_work()

What this is equivalent too, roughly:

Thread 1:

saved_warning_state_1 = global_warning_state
warnings.filterwarnings("ignore", category=UserWarning)  # changes global_warning_state
thread_1_work()
global_warning_state = saved_warning_state_1

Thread 2:

saved_warning_state_2 = global_warning_state
warnings.filterwarnings("ignore", category=OtherWarning)  # changes global_warning_state
thread_2_work()
global_warning_state = saved_warning_state_2

And these two run concurrently - for example:

saved_warning_state_1 = global_warning_state
warnings.filterwarnings("ignore", category=UserWarning)  # changes global_warning_state
thread_1_work()

saved_warning_state_2 = global_warning_state
global_warning_state = saved_warning_state_1

warnings.filterwarnings("ignore", category=OtherWarning)  # changes global_warning_state
thread_2_work()
global_warning_state = saved_warning_state_2

# Still ignoring UserWarning

nicoddemus commented 6 months ago

pytest is not multi-thread, so this is not a problem. Also, pytest itself uses catch_warnings to capture warnings, so it is safe.

bersbersbers commented 6 months ago

pytest is not multi-thread

That is true. But what if pytest is used to test multi-threaded code?

pytest-dev / pytest-qt

"RuntimeError: Failed to disconnect" with `PySide6==6.7.0` #552