From 69143c646f879c7c38504a22aad10fbcdb32eeec Mon Sep 17 00:00:00 2001 From: Catherine Date: Sat, 21 Oct 2023 15:36:40 +0000 Subject: [PATCH 1/4] support.task_queue: consider queues with only finished tasks non-empty. 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. --- software/glasgow/support/task_queue.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/software/glasgow/support/task_queue.py b/software/glasgow/support/task_queue.py index 64bb08c91..d38d0f824 100644 --- a/software/glasgow/support/task_queue.py +++ b/software/glasgow/support/task_queue.py @@ -82,12 +82,12 @@ async def wait_all(self): await self.poll() def __bool__(self): - """Check whether there are any pending tasks in the queue.""" - return bool(self._live) + """Check whether there are any tasks in the queue.""" + return bool(self._live) or bool(self._done) def __len__(self): - """Count pending tasks in the queue.""" - return len(self._live) + """Count tasks in the queue.""" + return len(self._live) + len(self._done) @property def total_wait_time(self): From fd0931c35de3e0ba431da8fd9d1762d2987036bc Mon Sep 17 00:00:00 2001 From: Catherine Date: Sat, 21 Oct 2023 15:53:30 +0000 Subject: [PATCH 2/4] support.task_queue: do not ignore task exception on cancellation. 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. --- software/glasgow/support/task_queue.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/software/glasgow/support/task_queue.py b/software/glasgow/support/task_queue.py index d38d0f824..e435f49a9 100644 --- a/software/glasgow/support/task_queue.py +++ b/software/glasgow/support/task_queue.py @@ -38,12 +38,25 @@ def submit(self, coro): async def cancel(self): """ Cancel all tasks in the queue, and wait until cancellation is finished. + + After cancelling them, waits until all the pending tasks become finished, and then awaits + every finished task, ignoring cancellation errors. If some of the tasks raised an exception + before or during being cancelled, the first exception will be re-raised, and the other ones + will be consumed. """ for task in self._live: task.cancel() if self._live: await asyncio.wait(self._live, return_when=asyncio.ALL_COMPLETED) - self._done.clear() + exception = None + while self._done: + task = self._done.popleft() + if task.cancelled(): + continue + if task.exception() is not None and exception is None: + exception = task.exception() + if exception is not None: + raise exception async def poll(self): """ From b402e985c9919612df856e7f31357045adbdf488 Mon Sep 17 00:00:00 2001 From: Catherine Date: Sat, 21 Oct 2023 16:10:20 +0000 Subject: [PATCH 3/4] device.hardware: improve handling of surprise disconnection. 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. --- software/glasgow/device/hardware.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/software/glasgow/device/hardware.py b/software/glasgow/device/hardware.py index 18b9cc39e..56953898e 100644 --- a/software/glasgow/device/hardware.py +++ b/software/glasgow/device/hardware.py @@ -254,23 +254,29 @@ def usb_callback(transfer): elif status == usb1.TRANSFER_STALL: result_future.set_exception(usb1.USBErrorPipe()) elif status == usb1.TRANSFER_NO_DEVICE: - result_future.set_exception(GlasgowDeviceError("device lost")) + result_future.set_exception(GlasgowDeviceError("device disconnected")) else: result_future.set_exception(GlasgowDeviceError( "transfer error: {}".format(usb1.libusb1.libusb_transfer_status(status)))) + def handle_usb_error(func): + try: + func() + except usb1.USBErrorNoDevice: + raise GlasgowDeviceError("device disconnected") from None + loop = asyncio.get_event_loop() transfer.setCallback(lambda transfer: loop.call_soon_threadsafe(usb_callback, transfer)) - transfer.submit() + handle_usb_error(lambda: transfer.submit()) try: return await result_future - except asyncio.CancelledError: - try: - transfer.cancel() - await cancel_future - except usb1.USBErrorNotFound: - pass # already finished, one way or another - raise + finally: + if result_future.cancelled(): + try: + handle_usb_error(lambda: transfer.cancel()) + await cancel_future + except usb1.USBErrorNotFound: + pass # already finished, one way or another async def control_read(self, request_type, request, value, index, length): logger.trace("USB: CONTROL IN type=%#04x request=%#04x " From 5e113b42f189450d82c20d4cd5c512b59aa56c92 Mon Sep 17 00:00:00 2001 From: Catherine Date: Sat, 21 Oct 2023 16:32:00 +0000 Subject: [PATCH 4/4] cli: retrieve exception of applet task before finishing other work. 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 --- software/glasgow/cli.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/software/glasgow/cli.py b/software/glasgow/cli.py index 4649217c9..96c909c50 100644 --- a/software/glasgow/cli.py +++ b/software/glasgow/cli.py @@ -681,6 +681,10 @@ async def wait_for_sigint(): task.cancel() await asyncio.wait(tasks, return_when=asyncio.ALL_COMPLETED) + # If the applet task has raised an exception, retrieve it here in case any of the await + # statements above will fail; if we don't, asyncio will unnecessarily complain. + applet_task.exception() + if do_trace: await device.write_register(target.analyzer.addr_done, 1) await analyzer_task