Fix and clean up timer handling in signal/callback blockers

- Always create the timeout `QTimer`, even if it's not going to be used.
- Connect the signal once in `_AbstractSignalBlocker`/`CallbackBlocker.__init__`
  and never disconnect it.
- When checking whether a timeout was set, simply check `self._timeout` instead of
  `self._timer`.

Before this change, we conditionally created a `QTimer` and deleted it again
when cleaning up. In addition to coming with hard to follow complexity, this
also caused multiple issues around this timer:

Warnings about disconnecting signal
===================================

In `AbstractSignalBlocker._cleanup`, we attempted to disconnect
`self._timer.timeout()` from `self._quit_loop_by_timeout`, under the condition that
a timer was created in `__init__` (i.e. `self.timeout` was not `0` or `None`).

However, the signal only got connected in `AbstractSignalBlocker.wait()`.
Thus, if `AbstractSignalBlocker` is used as a context manager with a timeout and
the signal is emitted inside it:

- `self._timer` is present
- The signal calls `self._quit_loop_by_signal()`
- `self._cleanup()` gets called from there
- That tries to disconnect `self._timer.timeout()` from `self._quit_loop_by_timeout()`
- Since `self.wait()` was never called, the signal was never connected
- Which then results in either an exception (PyQt), an internal SystemError
    (older PySide6) or a warning (newer PySide6).

In 560f565 and later
81b317c this was fixed by ignoring
`TypeError`/`RuntimeError`, but that turned out to not be sufficient for newer
PySide versions: #552, #558.

The proper fix for this is to not attempt to disconnect a signal that was never
connected, which makes all the PySide6 warnings go away (and thus we can run our
testsuite with `-Werror` now).

As a drive-by fix, this also removes the old `filterwarnings` mark definition,
which was introduced in 95fee8b (perhaps as a
stop-gap for older pytest versions?) but shouldn't be needed anymore (and is
unused since e338809).

AttributeError when a signal/callback is emitted from a different thread
========================================================================

If a signal is emitted from a thread, `_AbstractSignalBlocker._cleanup()` also gets
called from the non-main thread (Qt will complain: "QObject::killTimer: Timers
cannot be stopped from another thread").

If the signal emission just happened to happen between the None-check and
calling `.start()` in `wait()`, it failed with an `AttributeError`:

Main thread in `AbstractSignalBlocker.wait()`:

```python
if self._timer is not None:  # <--- this was true...
    self._timer.timeout.connect(self._quit_loop_by_timeout)
    # <--- ...now here, but self._timer is None now!
    self._timer.start()
````

Emitting thread in `AbstractSignalBlocker._cleanup()` via
`._quit_loop_by_signal()`:

```python
if self._timer is not None:
    ...
    self._timer = None  # <--- here
```

In SignalBlocker.connect, we used:

```python
actual_signal.connect(self._quit_loop_by_signal)
```

which by default is supposed to use a `QueuedConnection` if the signal gets
emitted in a different thread:

    https://doc.qt.io/qt-6/qt.html#ConnectionType-enum
    (Default) If the receiver lives in the thread that emits the signal,
    `Qt::DirectConnection` is used. Otherwise, `Qt::QueuedConnection` is used. The
    connection type is determined when the signal is emitted.

though then that page says:

    https://doc.qt.io/qt-6/qobject.html#thread-affinity
    Note: If a `QObject` has no thread affinity (that is, if `thread()` returns
    zero), or if it lives in a thread that has no running event loop, then it
    cannot receive queued signals or posted events.

Which means `AbstractSignalBlocker` needs to be a `QObject` for this to work.

However, that means we need to use `qt_api.QtCore.QObject` as subclass, i.e. at
import time of `wait_signal.py`. Yet, `qt_api` only gets initialized in
`pytest_configure` so that it's configurable via a pytest setting.

Unfortunately, doing that is tricky, as it means we can't import
`wait_signal.py` at all during import time, and people might do so for e.g. type
annotations.

With this refactoring, the `AttributeError` is out of the way, though there are
other subtle failures with multi-threaded signals now:

1) `_quit_loop_by_signal()` -> `_cleanup()` now simply calls `self._timer.stop()`
without setting `self._timer` to `None`. This still results in the same Qt
message quoted above (after all, the timer still doesn't belong to the calling
thread!), but it looks like the test terminates without any timeout anyways.
From what I can gather, while the "low level timer" continues to run (and
waste a minimal amount of resources), the QTimer still "detaches" from it and
stops running. The commit adds a test to catch this case (currently marked as
xfail).

2) The main thread in `wait()` can now still call `self._timer.start()` without
an `AttributeError`. However, in theory this could restart the timer after it
was already stopped by the signal emission, with a race between `_cleanup()` and
`wait()`.

See #586. This fixes the test-case posted by the reporter (added to the
testsuite in a simplified version), but not the additional considerations above.

The same fix is also applied to `CallbackBlocker`, though the test there is way
more unreliable in triggering the issue, and thus is skipped for taking too
long.

Author: The-Compiler

CHANGELOG.rst

@@ -1,9 +1,20 @@
 UNRELEASED
 ----------
 
-* Added official support for Python 3.13.
-* Dropped support for EOL Python 3.8.
-* Dropped support for EOL PySide 2.
+- Added official support for Python 3.13.
+- Dropped support for EOL Python 3.8.
+- Dropped support for EOL PySide 2.
+- Fixed PySide6 exceptions / warnings about being unable to disconnect signals
+  with ``qtbot.waitSignal`` (`#552`_, `#558`_).
+- Reduced the likelyhood of trouble when using ``qtbot.waitSignal(s)`` and
+  ``qtbot.waitCallback`` where the signal/callback is emitted from a non-main
+  thread. In theory, more problems remain and this isn't a proper fix yet. In
+  practice, it seems impossible to provoke any problems in pytest-qt's testsuite.
+  (`#586`_)
+
+..  _#552: https://github.com/pytest-dev/pytest-qt/issues/552
+..  _#558: https://github.com/pytest-dev/pytest-qt/issues/558
+..  _#586: https://github.com/pytest-dev/pytest-qt/issues/586
 
 4.4.0 (2024-02-07)
 ------------------

pytest.ini

@@ -2,5 +2,4 @@
 testpaths = tests
 addopts = --strict-markers --strict-config
 xfail_strict = true
-markers =
-  filterwarnings: pytest's filterwarnings marker
+filterwarnings = error

src/pytestqt/wait_signal.py

@@ -24,12 +24,12 @@ def __init__(self, timeout=5000, raising=True):
         self.raising = raising
         self._signals = None  # will be initialized by inheriting implementations
         self._timeout_message = ""
-        if timeout is None or timeout == 0:
-            self._timer = None
-        else:
-            self._timer = qt_api.QtCore.QTimer(self._loop)
-            self._timer.setSingleShot(True)
+
+        self._timer = qt_api.QtCore.QTimer(self._loop)
+        self._timer.setSingleShot(True)
+        if timeout is not None:
             self._timer.setInterval(timeout)
+        self._timer.timeout.connect(self._quit_loop_by_timeout)
 
     def wait(self):
         """
@@ -43,11 +43,13 @@ def wait(self):
             return
         if self.timeout is None and not self._signals:
             raise ValueError("No signals or timeout specified.")
-        if self._timer is not None:
-            self._timer.timeout.connect(self._quit_loop_by_timeout)
-            self._timer.start()
 
         if self.timeout != 0:
+            if self.timeout is not None:
+                # asserts as a stop-gap for possible multithreading issues
+                assert not self.signal_triggered
+                self._timer.start()
+                assert not self.signal_triggered
             qt_api.exec(self._loop)
 
         if not self.signal_triggered and self.raising:
@@ -62,10 +64,7 @@ def _quit_loop_by_timeout(self):
     def _cleanup(self):
         # store timeout message before the data to construct it is lost
         self._timeout_message = self._get_timeout_error_message()
-        if self._timer is not None:
-            _silent_disconnect(self._timer.timeout, self._quit_loop_by_timeout)
-            self._timer.stop()
-            self._timer = None
+        self._timer.stop()
 
     def _get_timeout_error_message(self):
         """Subclasses have to implement this, returning an appropriate error message for a TimeoutError."""
@@ -649,12 +648,12 @@ def __init__(self, timeout=5000, raising=True):
         self.kwargs = None
         self.called = False
         self._loop = qt_api.QtCore.QEventLoop()
-        if timeout is None:
-            self._timer = None
-        else:
-            self._timer = qt_api.QtCore.QTimer(self._loop)
-            self._timer.setSingleShot(True)
+
+        self._timer = qt_api.QtCore.QTimer(self._loop)
+        self._timer.setSingleShot(True)
+        if timeout is not None:
             self._timer.setInterval(timeout)
+        self._timer.timeout.connect(self._quit_loop_by_timeout)
 
     def wait(self):
         """
@@ -664,8 +663,7 @@ def wait(self):
         __tracebackhide__ = True
         if self.called:
             return
-        if self._timer is not None:
-            self._timer.timeout.connect(self._quit_loop_by_timeout)
+        if self.timeout is not None:
             self._timer.start()
         qt_api.exec(self._loop)
         if not self.called and self.raising:
@@ -687,10 +685,7 @@ def _quit_loop_by_timeout(self):
             self._loop.quit()
 
     def _cleanup(self):
-        if self._timer is not None:
-            _silent_disconnect(self._timer.timeout, self._quit_loop_by_timeout)
-            self._timer.stop()
-            self._timer = None
+        self._timer.stop()
 
     def __call__(self, *args, **kwargs):
         # Not inside the try: block, as if self.called is True, we did quit the

tests/test_wait_signal.py

@@ -1364,3 +1364,132 @@ def test_timeout_not_raising(self, qtbot):
         assert not callback.called
         assert callback.args is None
         assert callback.kwargs is None
+
+
+@pytest.mark.parametrize(
+    "check_stderr, count",
+    [
+        # Checking stderr messages
+        pytest.param(
+            True,  # check stderr
+            200,  # gets output reliably even with only few runs (often the first)
+            id="stderr",
+        ),
+        # Triggering AttributeError
+        pytest.param(
+            False,  # don't check stderr
+            # Hopefully enough to trigger the AttributeError race condition reliably.
+            # With 500 runs, only 1 of 5 Windows PySide6 CI jobs triggered it (but all
+            # Ubuntu/macOS jobs did).  With 1500 runs, Windows jobs still only triggered
+            # it 0-2 times.
+            #
+            # On my machine (Linux, Intel Core Ultra 9 185H), 500 runs trigger it
+            # reliably and take ~1s in total.
+            2500 if sys.platform == "win32" else 500,
+            id="attributeerror",
+        ),
+    ],
+)
+@pytest.mark.parametrize("multi_blocker", [True, False])
+def test_signal_raised_from_thread(
+    pytester: pytest.Pytester, check_stderr: bool, multi_blocker: bool, count: int
+) -> None:
+    """Wait for a signal with a thread.
+
+    Extracted from https://github.com/pytest-dev/pytest-qt/issues/586
+    """
+    pytester.makepyfile(
+        f"""
+        import pytest
+        from pytestqt.qt_compat import qt_api
+
+
+        class Worker(qt_api.QtCore.QObject):
+            signal = qt_api.Signal()
+
+
+        @pytest.mark.parametrize("_", range({count}))
+        def test_thread(qtbot, capfd, _):
+            worker = Worker()
+            thread = qt_api.QtCore.QThread()
+            worker.moveToThread(thread)
+            thread.start()
+
+            try:
+                if {multi_blocker}:  # multi_blocker
+                    with qtbot.waitSignals([worker.signal], timeout=500) as blocker:
+                        worker.signal.emit()
+                else:
+                    with qtbot.waitSignal(worker.signal, timeout=500) as blocker:
+                        worker.signal.emit()
+            finally:
+                thread.quit()
+                thread.wait()
+
+            if {check_stderr}:  # check_stderr
+                out, err = capfd.readouterr()
+                assert not err
+    """
+    )
+
+    res = pytester.runpytest_subprocess("-x", "-s")
+    outcomes = res.parseoutcomes()
+
+    if outcomes.get("failed", 0) and check_stderr and qt_api.pytest_qt_api == "pyside6":
+        # The test succeeds on PyQt (unsure why!), but we can't check
+        # qt_api.pytest_qt_api at import time, so we can't use
+        # pytest.mark.xfail conditionally.
+        pytest.xfail(
+            "Qt error: QObject::killTimer: "
+            "Timers cannot be stopped from another thread"
+        )
+
+    res.assert_outcomes(passed=outcomes["passed"])  # no failed/error
+
+
+@pytest.mark.skip(reason="Runs ~1min to reproduce bug reliably")
+def test_callback_in_thread(pytester: pytest.Pytester) -> None:
+    """Wait for a callback with a thread.
+
+    Inspired by https://github.com/pytest-dev/pytest-qt/issues/586
+    """
+    # Hopefully enough to trigger the bug reliably.
+    #
+    # On my machine (Linux, Intel Core Ultra 9 185H), sometimes the bug only
+    # triggers after ~30k runs (~44s). Thus, we skip this test by default.
+    count = 50_000
+
+    pytester.makepyfile(
+        f"""
+        import pytest
+        from pytestqt.qt_compat import qt_api
+
+
+        class Worker(qt_api.QtCore.QObject):
+            def __init__(self, callback):
+                super().__init__()
+                self.callback = callback
+
+            def call_callback(self):
+                self.callback()
+
+
+        @pytest.mark.parametrize("_", range({count}))
+        def test_thread(qtbot, _):
+            thread = qt_api.QtCore.QThread()
+
+            try:
+                with qtbot.waitCallback() as callback:
+                    worker = Worker(callback)
+                    worker.moveToThread(thread)
+                    thread.started.connect(worker.call_callback)
+                    thread.start()
+            finally:
+                thread.quit()
+                thread.wait()
+    """
+    )
+
+    res = pytester.runpytest_subprocess("-x")
+    outcomes = res.parseoutcomes()
+    res.assert_outcomes(passed=outcomes["passed"])  # no failed/error