Do not fill terminal with noise when a device is surprise disconnected #452
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
Before this commit, a queue with only finished tasks was considered empty. This is problematic because the finished tasks still carry their result value and, more visibly, exceptions; if not retrieved, asyncio will complain about tasks with exceptions on interpreter shutdown. After this commit, any tasks in a queue make it non-empty. (That is, casting to bool will make the queue truthful, and the length includes every task.) The only user of these API calls is the demultiplexer, and it only uses them in one call site, which is improved by the change.
Before this commit, cancelling a task queue would cancel all the tasks
and, once they finish, clear them from the queue, ignoring their result
or exception. This isn't correct since it ignores errors that could have
been raised before or during cancellation, and it also causes asyncio to
print a *lot* of backtraces (one per task) on interpreter shutdown.
After this commit, all tasks are awaited, and then the first exception
that isn't asyncio.CancelledError is re-raised. As a result, unplugging
a device while it's open results in a single error message...
T: g.applet.radio.sx1276: FIFO: cancelling operations
E: g.cli: device lost
... rather than sixteen backtraces.
However, more work is necessary to handle surprise disconnection while
there are ongoing transfers.
This commit adds handling of the LIBUSB_ERROR_NO_DEVICE error during transfer submission. This error was already handled during transfer completion, but since surprise disconnection is (by definition) asynchronous, this it can happen right before submission too. The message is also changed from ambiguous "device lost" (lost where?) to unambiguous "device disconnected". The code in _do_transfer with the exception handler nested three levels deep is some of the most cursed and convoluted logic I have ever written but as far as I can tell it is reasonably appropriate to the task. Sigh. This is still not quite enough to reduce the errors printed for a surprise disconnection to a single line, but we're getting there.
Otherwise asyncio will print a very long backtrace corresponding to
the applet task (without any useful information) if, most commonly,
the device was surprise unplugged and all of the other tasks that need
USB communication fail.
After this commit, finally, this works as it should:
$ glasgow run benchmark -c 1000000000
I: g.device.hardware: generating bitstream ID 6b120f4f94cf4ccd95c51417abc09069
I: g.cli: running handler for applet 'benchmark'
I: g.applet.internal.benchmark: running benchmark mode source for 953.674 MiB
E: g.cli: device disconnected
whitequark
requested review from
mwkmwkmwk and
attie
and removed request for
mwkmwkmwk
|
Oh, hang on...
I see: |
This is also fine--the |
|
The difference is basically that in one case a planned transfer was never scheduled because the device was gone, and in another a scheduled transfer failed because the device went poof while it was in progress. Not much more I can do. |
I was just digging in to it too... it's not as shiny as a "device disconnected", but I agree it's the best we can do. |
Merged
via the queue into
GlasgowEmbedded:main
with commit Oct 21, 2023
21c4f66
10 of 12 checks passed
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.
"Surprise disconnected" means disconnecting a device while it is being used by an applet, script, or REPL; whether there are ongoing transfers or not.
After this commit, in general, only one message should be printed:
However, applet logic will often not handle cancellation or USB transfer errors, so certain individual applets may still print more noise.
This has been one of the more irritating papercuts, especially as all that noise will make recovering from an accidentally unplugged device all that more annoying, and it's confusing to people who have not seen the error before too.
See individual commit messages for details of implementation, which is nightmarish.
Fixes #386.