WIP: Make AbstractSignalBlocker a QObject and fix signals in threads
#594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The-Compiler
force-pushed
the
signal-blocker-qobject
branch
3 times, most recently
from
620a3de to
cd5af92
Compare
- 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.
After enabling -Werror for tests, for unknown reasons,
tests/test_wait_signal.py::test_zero_timeout[True] consistently fails on
macOS + Python 3.13 + PySide6 (only there!) with:
RuntimeWarning: Failed to disconnect (functools.partial(<bound method
timer.<locals>.Timer._emit of <conftest.Timer(0x...) at 0x...>>,
<PySide6.QtCore.SignalInstance signal() at 0x...>)) from signal "timeout()".
Qt supports connecting signals to signals, so we can save the detour via Python
and functools.partial.
…ions Preparation for avoiding early wait_signal.py imports, but also seems more consistent.
With this, we can inherit from qt_api.QtCore.QObject without needing to entirely avoid importing wait_signal.py at initial import time.
The-Compiler
force-pushed
the
signal-blocker-qobject
branch
2 times, most recently
from
306f957 to
dd0f762
Compare
The-Compiler
force-pushed
the
signal-blocker-qobject
branch
from
34d605f to
32ef058
Compare
for more information, see https://pre-commit.ci
This was referenced Mar 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only pushing test for now to see it fail on CI.