Fix and clean up timer handling in signal/callback blockers

[The-Compiler]

• 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():

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():

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

In SignalBlocker.connect, we used:

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.

[The-Compiler]

Ran into some unexpected issues on CI, but now all should be fine!

[nicoddemus]

Awesome work!

[The-Compiler]

Looks like all is green and ready finally 😅

Should I self-merge?

[nicoddemus]

Should I self-merge?

Sure please go ahead!