Catch DisconnectedError in create_task wrapper instead of per-coroutine#806
Open
gemenerik wants to merge 3 commits intoAris/cflib2/DisconnectedErrorfrom
Open
Catch DisconnectedError in create_task wrapper instead of per-coroutine#806gemenerik wants to merge 3 commits intoAris/cflib2/DisconnectedErrorfrom
gemenerik wants to merge 3 commits intoAris/cflib2/DisconnectedErrorfrom
Conversation
PySide6's QtAsyncio prints tracebacks for unhandled task exceptions before done callbacks run, so the existing _task_done_callback catch was too late. Wrap coroutines in create_task to catch DisconnectedError before it reaches PySide6, and remove the now-redundant per-coroutine except blocks.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes how DisconnectedError is handled for background asyncio tasks to avoid PySide6/QtAsyncio printing “unhandled task exception” tracebacks before task done-callbacks run, by catching DisconnectedError inside the create_task() wrapper rather than inside each streaming coroutine.
Changes:
- Wrap all
create_task()coroutines with_wrap_disconnected()to swallowDisconnectedErrorbefore it escapes the coroutine. - Remove now-redundant per-coroutine
except DisconnectedError: passblocks from streaming loops. - Remove the dead
DisconnectedErrorhandling branch from_task_done_callback().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/cfclient/gui.py |
Adds coroutine wrapper to swallow DisconnectedError before QtAsyncio can log it; simplifies done-callback handling. |
src/cfclient/ui/tabs/FlightTab.py |
Removes redundant DisconnectedError swallowing from motor-streaming loop. |
src/cfclient/ui/pose_logger.py |
Removes redundant DisconnectedError swallowing from pose-streaming loop. |
src/cfclient/ui/main.py |
Removes redundant DisconnectedError swallowing from battery-streaming loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makes it possible to identify which task was interrupted when multiple background tasks are torn down by a single disconnect.
DisconnectedError is intentionally swallowed by the wrapper as a normal shutdown case; the original docstring suggested all exceptions propagate.
This file contains hidden or 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
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.
The existing
_task_done_callbackwas meant to catchDisconnectedErrorfor all tasks created viacreate_task, but PySide6's QtAsyncio logs unhandled task exceptions before done callbacks run. So the callback did catch the error (preventingos._exit), but the traceback was already printed. By wrapping the coroutine insidecreate_task, the exception is caught before it ever leaves the coroutine, so PySide6 never sees it. This also removes the now-redundant per-coroutineexcept DisconnectedError: passblocks and the deadexcept DisconnectedErrorfrom_task_done_callback.