fix(transport): usb api in node and abort

trezor · Sep 3, 2024 · 5c2cb7d · 5c2cb7d
1 parent f45938d
commit 5c2cb7d
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 18 deletions.
diff --git a/packages/transport-test/e2e/api/api.test.ts b/packages/transport-test/e2e/api/api.test.ts
@@ -1,6 +1,6 @@
 import { UsbApi } from '@trezor/transport/src/api/usb';
 
-import { buildMessage, assertMessage, assertSuccess, assertEquals } from './utils';
+import { buildMessage, assertMessage, assertSuccess, assertFailure, assertEquals } from './utils';
 import { sharedTest, success, info, debug, error } from './shared';
 
 /**
@@ -114,8 +114,11 @@ const runTests = async () => {
             const readPromise = api.read(path, abortController.signal);
             debug('trigger abort');
             abortController.abort();
-            await readPromise;
-            debug('write PING');
+            const abortedResponse = await readPromise;
+            assertFailure(abortedResponse);
+            assertEquals(abortedResponse.error, 'Aborted by signal');
+
+            debug('write PING', readPromise);
             await api.write(path, buildMessage('PING'));
             debug('read and expect SUCCESS');
             const readResponse = await api.read(path);

diff --git a/packages/transport-test/e2e/api/utils.ts b/packages/transport-test/e2e/api/utils.ts
@@ -26,7 +26,12 @@ export const assertMessage = (message: Buffer, expected: keyof typeof MESSAGES)
 };
 export function assertSuccess(result: any): asserts result is { success: true; payload: any } {
     if (!result.success) {
-        throw new Error(error(result.error));
+        throw new Error(error(`${result.error}${result.message ? `: ${result.message}` : ''}`));
+    }
+}
+export function assertFailure(result: any): asserts result is { success: false; error: any } {
+    if (result.success) {
+        throw new Error(error(`Unexpected success`));
     }
 }
 export const assertEquals = (a: any, b: any) => {

diff --git a/packages/transport/src/api/usb.ts b/packages/transport/src/api/usb.ts
@@ -128,8 +128,8 @@ export class UsbApi extends AbstractApi {
     }
 
     private abortableMethod<R>(
-        method: () => R,
-        { signal, onAbort }: { signal?: AbortSignal; onAbort?: () => void },
+        method: () => Promise<R>,
+        { signal, onAbort }: { signal?: AbortSignal; onAbort?: () => Promise<void> | void },
     ) {
         if (!signal) {
             return method();
@@ -138,23 +138,29 @@ export class UsbApi extends AbstractApi {
             return Promise.reject(new Error(ERRORS.ABORTED_BY_SIGNAL));
         }
 
-        const abort = () => {
-            if (!onAbort) return;
-            try {
-                onAbort();
-            } catch (err) {
-                this.logger?.error(`usb: onAbort error: ${err}`);
-            }
-        };
-
         const dfd = createDeferred<R>();
-        const abortListener = () => {
-            abort();
+        const abortListener = async () => {
+            this.logger?.debug('usb: abortableMethod onAbort start');
+            try {
+                await onAbort?.();
+            } catch {}
+            this.logger?.debug('usb: abortableMethod onAbort done');
             dfd.reject(new Error(ERRORS.ABORTED_BY_SIGNAL));
         };
         signal?.addEventListener('abort', abortListener);
 
-        return Promise.race([method(), dfd.promise])
+        const methodPromise = method().catch(error => {
+            // NOTE: race condition
+            // method() rejects error triggered by signal (device.reset) before dfd.promise (before onAbort finish)
+            this.logger?.debug(`usb: abortableMethod method() aborted: ${signal.aborted} ${error}`);
+            if (signal.aborted) {
+                return dfd.promise;
+            }
+            dfd.reject(error);
+            throw error;
+        });
+
+        return Promise.race([methodPromise, dfd.promise])
             .then(r => {
                 dfd.resolve(r);
 
@@ -360,6 +366,10 @@ export class UsbApi extends AbstractApi {
             try {
                 const interfaceId = this.debugLink ? DEBUGLINK_INTERFACE_ID : INTERFACE_ID;
                 this.logger?.debug(`usb: device.releaseInterface: ${interfaceId}`);
+                if (!this.debugLink) {
+                    // NOTE: `device.reset()` interrupts transfers for all interfaces (debugLink and normal)
+                    await device.reset();
+                }
                 await device.releaseInterface(interfaceId);
                 this.logger?.debug(
                     `usb: device.releaseInterface done: ${interfaceId}. device: ${this.formatDeviceForLog(device)}`,