SystemError on disconnect with pyside 6.7.1 #558
Comments
|
|
|
k |
|
would it be trivial for you to suggest an MRE without using pytest-qt's |
I don't have the time to work on it right now, but I suspect if you just connect a signal and try to disconnect it twice you will get the |
|
If you do report it upstream, please link it here for reference. 👍 |
|
PYSIDE-2705 suggests that the exception-like warning in 6.7.0 has been converted to an actual warning, compare https://codereview.qt-project.org/c/pyside/pyside-setup/+/558142/3/sources/pyside6/libpyside/pysidesignal.cpp. So there should be no more (unhandled) I was able to suppress these errors like this ( [tool.pytest.ini_options]
filterwarnings = [
# https://github.com/pytest-dev/pytest-qt/issues/558
"ignore:Failed to disconnect .* from signal:RuntimeWarning",
]That gets rid of |
|
ahh, thank you for noting that. indeed I turn my warnings into exceptions, and I failed to note that this was just a warning and not an actual exception. adding that ignore works fine for my case as well. I'll close this issue, but @nicoddemus feel free to reopen if you want to track anything else related to this change |
The-Compiler
added a commit
that referenced
this issue
Mar 22, 2025
- 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.
The-Compiler
added a commit
that referenced
this issue
Mar 22, 2025
- 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.
|
This should be fixed properly with #596. FWIW the PySide6 fix seems incomplete too, as with |
The-Compiler
added a commit
that referenced
this issue
Mar 22, 2025
- 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.
The-Compiler
added a commit
that referenced
this issue
Mar 22, 2025
- 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.
The-Compiler
added a commit
that referenced
this issue
Mar 22, 2025
- 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.
The-Compiler
added a commit
that referenced
this issue
Mar 22, 2025
- 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.
The-Compiler
added a commit
that referenced
this issue
Mar 25, 2025
- 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Don't see an existing issue for this yet, so opening one. This seems related to #552, but has now turned into an exception with pyside 6.7.1:
running this test raises:
on Pyside 6.7.0, this worked but caused the RuntimeError warning as noted in #552.
The text was updated successfully, but these errors were encountered: