From 700a3c0e0977e425212c35220aedbf350cd5afa8 Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Mon, 13 Dec 2021 06:36:02 +0100 Subject: [PATCH 1/4] feat: Support abort reasons in Deno APIs and `WebSocketStream` --- cli/tests/testdata/websocketstream_test.ts | 49 ++++++++++++- cli/tests/unit/read_file_test.ts | 46 +++++++++++- cli/tests/unit/read_text_file_test.ts | 46 +++++++++++- cli/tests/unit/write_file_test.ts | 83 ++++++++++++++++++++++ ext/websocket/02_websocketstream.js | 12 ++-- ext/websocket/lib.rs | 5 +- runtime/js/12_io.js | 15 ++-- runtime/js/40_write_file.js | 21 +++--- 8 files changed, 242 insertions(+), 35 deletions(-) diff --git a/cli/tests/testdata/websocketstream_test.ts b/cli/tests/testdata/websocketstream_test.ts index ba5c281ebe7e68..1198c416417845 100644 --- a/cli/tests/testdata/websocketstream_test.ts +++ b/cli/tests/testdata/websocketstream_test.ts @@ -1,9 +1,11 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. import { + assert, assertEquals, assertRejects, assertThrows, + unreachable, } from "../../../test_util/std/testing/asserts.ts"; Deno.test("fragment", () => { @@ -89,8 +91,49 @@ Deno.test("aborting immediately throws an AbortError", async () => { controller.abort(); await assertRejects( () => wss.connection, - DOMException, - "connection was aborted", + (error: Error) => { + assert(error instanceof DOMException); + assertEquals(error.name, "AbortError"); + }, + ); + await assertRejects( + () => wss.closed, + (error: Error) => { + assert(error instanceof DOMException); + assertEquals(error.name, "AbortError"); + }, + ); +}); + +Deno.test("aborting immediately with a reason throws that reason", async () => { + const controller = new AbortController(); + const wss = new WebSocketStream("ws://localhost:4242", { + signal: controller.signal, + }); + const abortReason = new Error(); + controller.abort(abortReason); + await assertRejects( + () => wss.connection, + (error: Error) => assertEquals(error, abortReason), + ); + await assertRejects( + () => wss.closed, + (error: Error) => assertEquals(error, abortReason), + ); +}); + +Deno.test("aborting immediately with a primitive as reason throws that primitive", async () => { + const controller = new AbortController(); + const wss = new WebSocketStream("ws://localhost:4242", { + signal: controller.signal, + }); + controller.abort("Some string"); + await wss.connection.then( + () => unreachable(), + (e) => assertEquals(e, "Some string"), + ); + await wss.closed.then( + () => unreachable(), + (e) => assertEquals(e, "Some string"), ); - await assertRejects(() => wss.closed, DOMException, "connection was aborted"); }); diff --git a/cli/tests/unit/read_file_test.ts b/cli/tests/unit/read_file_test.ts index df3161f0b21f9d..4b1ebef0bd7fb9 100644 --- a/cli/tests/unit/read_file_test.ts +++ b/cli/tests/unit/read_file_test.ts @@ -6,6 +6,7 @@ import { assertRejects, assertThrows, pathToAbsoluteFileUrl, + unreachable, } from "./test_util.ts"; Deno.test({ permissions: { read: true } }, function readFileSyncSuccess() { @@ -95,11 +96,52 @@ Deno.test( async function readFileWithAbortSignal() { const ac = new AbortController(); queueMicrotask(() => ac.abort()); - await assertRejects(async () => { + await assertRejects( + async () => { + await Deno.readFile("cli/tests/testdata/fixture.json", { + signal: ac.signal, + }); + }, + (error: Error) => { + assert(error instanceof DOMException); + assertEquals(error.name, "AbortError"); + }, + ); + }, +); + +Deno.test( + { permissions: { read: true } }, + async function readFileWithAbortSignalReason() { + const ac = new AbortController(); + const abortReason = new Error(); + queueMicrotask(() => ac.abort(abortReason)); + await assertRejects( + async () => { + await Deno.readFile("cli/tests/testdata/fixture.json", { + signal: ac.signal, + }); + }, + (error: Error) => { + assertEquals(error, abortReason); + }, + ); + }, +); + +Deno.test( + { permissions: { read: true } }, + async function readFileWithAbortSignalPrimitiveReason() { + const ac = new AbortController(); + queueMicrotask(() => ac.abort("Some string")); + try { await Deno.readFile("cli/tests/testdata/fixture.json", { signal: ac.signal, }); - }); + unreachable(); + } catch (e) { + assertEquals(e, "Some string"); + } }, ); diff --git a/cli/tests/unit/read_text_file_test.ts b/cli/tests/unit/read_text_file_test.ts index 6e7dce73edf444..169972cb42cbdb 100644 --- a/cli/tests/unit/read_text_file_test.ts +++ b/cli/tests/unit/read_text_file_test.ts @@ -4,6 +4,7 @@ import { assertRejects, assertThrows, pathToAbsoluteFileUrl, + unreachable, } from "./test_util.ts"; Deno.test({ permissions: { read: true } }, function readTextFileSyncSuccess() { @@ -88,11 +89,52 @@ Deno.test( async function readTextFileWithAbortSignal() { const ac = new AbortController(); queueMicrotask(() => ac.abort()); - await assertRejects(async () => { + await assertRejects( + async () => { + await Deno.readFile("cli/tests/testdata/fixture.json", { + signal: ac.signal, + }); + }, + (error: Error) => { + assert(error instanceof DOMException); + assertEquals(error.name, "AbortError"); + }, + ); + }, +); + +Deno.test( + { permissions: { read: true } }, + async function readTextFileWithAbortSignalReason() { + const ac = new AbortController(); + const abortReason = new Error(); + queueMicrotask(() => ac.abort(abortReason)); + await assertRejects( + async () => { + await Deno.readFile("cli/tests/testdata/fixture.json", { + signal: ac.signal, + }); + }, + (error: Error) => { + assertEquals(error, abortReason); + }, + ); + }, +); + +Deno.test( + { permissions: { read: true } }, + async function readTextFileWithAbortSignalPrimitiveReason() { + const ac = new AbortController(); + queueMicrotask(() => ac.abort("Some string")); + try { await Deno.readFile("cli/tests/testdata/fixture.json", { signal: ac.signal, }); - }); + unreachable(); + } catch (e) { + assertEquals(e, "Some string"); + } }, ); diff --git a/cli/tests/unit/write_file_test.ts b/cli/tests/unit/write_file_test.ts index 66f07b9b1cd521..b6f7d2ceea7833 100644 --- a/cli/tests/unit/write_file_test.ts +++ b/cli/tests/unit/write_file_test.ts @@ -4,6 +4,7 @@ import { assertEquals, assertRejects, assertThrows, + unreachable, } from "./test_util.ts"; Deno.test( @@ -250,6 +251,7 @@ Deno.test( queueMicrotask(() => ac.abort()); try { await Deno.writeFile(filename, data, { signal: ac.signal }); + unreachable(); } catch (e) { assert(e instanceof Error); assertEquals(e.name, "AbortError"); @@ -259,6 +261,45 @@ Deno.test( }, ); +Deno.test( + { permissions: { read: true, write: true } }, + async function writeFileAbortSignalReason(): Promise { + const ac = new AbortController(); + const enc = new TextEncoder(); + const data = enc.encode("Hello"); + const filename = Deno.makeTempDirSync() + "/test.txt"; + const abortReason = new Error(); + queueMicrotask(() => ac.abort(abortReason)); + try { + await Deno.writeFile(filename, data, { signal: ac.signal }); + unreachable(); + } catch (e) { + assertEquals(e, abortReason); + } + const stat = Deno.statSync(filename); + assertEquals(stat.size, 0); + }, +); + +Deno.test( + { permissions: { read: true, write: true } }, + async function writeFileAbortSignalPrimitiveReason(): Promise { + const ac = new AbortController(); + const enc = new TextEncoder(); + const data = enc.encode("Hello"); + const filename = Deno.makeTempDirSync() + "/test.txt"; + queueMicrotask(() => ac.abort("Some string")); + try { + await Deno.writeFile(filename, data, { signal: ac.signal }); + unreachable(); + } catch (e) { + assertEquals(e, "Some string"); + } + const stat = Deno.statSync(filename); + assertEquals(stat.size, 0); + }, +); + Deno.test( { permissions: { read: true, write: true } }, async function writeFileAbortSignalPreAborted(): Promise { @@ -269,6 +310,7 @@ Deno.test( const filename = Deno.makeTempDirSync() + "/test.txt"; try { await Deno.writeFile(filename, data, { signal: ac.signal }); + unreachable(); } catch (e) { assert(e instanceof Error); assertEquals(e.name, "AbortError"); @@ -277,3 +319,44 @@ Deno.test( assertEquals(stat.size, 0); }, ); + +Deno.test( + { permissions: { read: true, write: true } }, + async function writeFileAbortSignalReasonPreAborted(): Promise { + const ac = new AbortController(); + const abortReason = new Error(); + ac.abort(abortReason); + const enc = new TextEncoder(); + const data = enc.encode("Hello"); + const filename = Deno.makeTempDirSync() + "/test.txt"; + try { + await Deno.writeFile(filename, data, { signal: ac.signal }); + unreachable(); + } catch (e) { + assertEquals(e, abortReason); + } + const stat = Deno.statSync(filename); + assertEquals(stat.size, 0); + }, +); + +Deno.test( + { permissions: { read: true, write: true } }, + async function writeFileAbortSignalPrimitiveReasonPreAborted(): Promise< + void + > { + const ac = new AbortController(); + ac.abort("Some string"); + const enc = new TextEncoder(); + const data = enc.encode("Hello"); + const filename = Deno.makeTempDirSync() + "/test.txt"; + try { + await Deno.writeFile(filename, data, { signal: ac.signal }); + unreachable(); + } catch (e) { + assertEquals(e, "Some string"); + } + const stat = Deno.statSync(filename); + assertEquals(stat.size, 0); + }, +); diff --git a/ext/websocket/02_websocketstream.js b/ext/websocket/02_websocketstream.js index 246ddb1e3b7e77..dba22d557aa3f0 100644 --- a/ext/websocket/02_websocketstream.js +++ b/ext/websocket/02_websocketstream.js @@ -125,10 +125,7 @@ if (options.signal?.aborted) { core.close(cancelRid); - const err = new DOMException( - "This operation was aborted", - "AbortError", - ); + const err = options.signal.reason; this[_connection].reject(err); this[_closed].reject(err); } else { @@ -313,7 +310,12 @@ } }, (err) => { - core.tryClose(cancelRid); + if (err instanceof core.Interrupted) { + // The signal was aborted. + err = options.signal.reason; + } else { + core.tryClose(cancelRid); + } this[_connection].reject(err); this[_closed].reject(err); }, diff --git a/ext/websocket/lib.rs b/ext/websocket/lib.rs index 13d3ddfca8d659..91e574c16c7de0 100644 --- a/ext/websocket/lib.rs +++ b/ext/websocket/lib.rs @@ -298,10 +298,7 @@ where let client = client_async(request, socket); let (stream, response): (WsStream, Response) = if let Some(cancel_resource) = cancel_resource { - client - .or_cancel(cancel_resource.0.to_owned()) - .await - .map_err(|_| DomExceptionAbortError::new("connection was aborted"))? + client.or_cancel(cancel_resource.0.to_owned()).await? } else { client.await } diff --git a/runtime/js/12_io.js b/runtime/js/12_io.js index 1dd1629653adeb..213e0a1eefc03e 100644 --- a/runtime/js/12_io.js +++ b/runtime/js/12_io.js @@ -7,7 +7,6 @@ ((window) => { const core = window.Deno.core; - const { DOMException } = window.__bootstrap.domException; const { Uint8Array, ArrayPrototypePush, @@ -123,7 +122,8 @@ async function readAllInner(r, options) { const buffers = []; const signal = options?.signal ?? null; - while (!signal?.aborted) { + while (true) { + signal?.throwIfAborted(); const buf = new Uint8Array(READ_PER_ITER); const read = await r.read(buf); if (typeof read == "number") { @@ -132,9 +132,7 @@ break; } } - if (signal?.aborted) { - throw new DOMException("The read operation was aborted.", "AbortError"); - } + signal?.throwIfAborted(); return concatBuffers(buffers); } @@ -200,7 +198,8 @@ const buf = new Uint8Array(size + 1); // 1B to detect extended files let cursor = 0; const signal = options?.signal ?? null; - while (!signal?.aborted && cursor < size) { + while (cursor < size) { + signal?.throwIfAborted(); const sliceEnd = MathMin(size + 1, cursor + READ_PER_ITER); const slice = buf.subarray(cursor, sliceEnd); const read = await r.read(slice); @@ -210,9 +209,7 @@ break; } } - if (signal?.aborted) { - throw new DOMException("The read operation was aborted.", "AbortError"); - } + signal?.throwIfAborted(); // Handle truncated or extended files during read if (cursor > size) { diff --git a/runtime/js/40_write_file.js b/runtime/js/40_write_file.js index 6ced4e06690ab0..621d8144e3554b 100644 --- a/runtime/js/40_write_file.js +++ b/runtime/js/40_write_file.js @@ -13,9 +13,7 @@ data, options = {}, ) { - if (options?.signal?.aborted) { - throw new DOMException("The write operation was aborted.", "AbortError"); - } + options.signal?.throwIfAborted(); if (options.create !== undefined) { const create = !!options.create; if (!create) { @@ -73,15 +71,18 @@ const signal = options?.signal ?? null; let nwritten = 0; - while (!signal?.aborted && nwritten < data.length) { - nwritten += await file.write(TypedArrayPrototypeSubarray(data, nwritten)); + try { + while (nwritten < data.length) { + signal?.throwIfAborted(); + nwritten += await file.write( + TypedArrayPrototypeSubarray(data, nwritten), + ); + } + } finally { + file.close(); } - file.close(); - - if (signal?.aborted) { - throw new DOMException("The write operation was aborted.", "AbortError"); - } + signal?.throwIfAborted(); } function writeTextFileSync( From 1fdde38435ba6c33aac2b217951b99c0263d913f Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Mon, 13 Dec 2021 08:57:27 +0100 Subject: [PATCH 2/4] Remove the `DomExceptionAbortError` error binding. --- ext/websocket/lib.rs | 26 -------------------------- runtime/errors.rs | 1 - 2 files changed, 27 deletions(-) diff --git a/ext/websocket/lib.rs b/ext/websocket/lib.rs index 91e574c16c7de0..4796eddc650132 100644 --- a/ext/websocket/lib.rs +++ b/ext/websocket/lib.rs @@ -505,29 +505,3 @@ pub fn get_network_error_class_name(e: &AnyError) -> Option<&'static str> { e.downcast_ref::() .map(|_| "DOMExceptionNetworkError") } - -#[derive(Debug)] -pub struct DomExceptionAbortError { - pub msg: String, -} - -impl DomExceptionAbortError { - pub fn new(msg: &str) -> Self { - DomExceptionAbortError { - msg: msg.to_string(), - } - } -} - -impl fmt::Display for DomExceptionAbortError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.pad(&self.msg) - } -} - -impl std::error::Error for DomExceptionAbortError {} - -pub fn get_abort_error_class_name(e: &AnyError) -> Option<&'static str> { - e.downcast_ref::() - .map(|_| "DOMExceptionAbortError") -} diff --git a/runtime/errors.rs b/runtime/errors.rs index 1491161d35c83a..c16572b5a9ed5d 100644 --- a/runtime/errors.rs +++ b/runtime/errors.rs @@ -158,7 +158,6 @@ pub fn get_error_class_name(e: &AnyError) -> Option<&'static str> { .or_else(|| deno_web::get_error_class_name(e)) .or_else(|| deno_webstorage::get_not_supported_error_class_name(e)) .or_else(|| deno_websocket::get_network_error_class_name(e)) - .or_else(|| deno_websocket::get_abort_error_class_name(e)) .or_else(|| { e.downcast_ref::() .map(get_dlopen_error_class) From 66152a94c2112bad1ba1d2d52fc0a296430345e9 Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Mon, 13 Dec 2021 11:33:54 +0100 Subject: [PATCH 3/4] review feedback --- runtime/js/40_write_file.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/runtime/js/40_write_file.js b/runtime/js/40_write_file.js index 621d8144e3554b..bb3f91789f031d 100644 --- a/runtime/js/40_write_file.js +++ b/runtime/js/40_write_file.js @@ -81,8 +81,6 @@ } finally { file.close(); } - - signal?.throwIfAborted(); } function writeTextFileSync( From 0202acd5920ca1e79f6c8ef2ce72faf984314f26 Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Mon, 13 Dec 2021 11:36:39 +0100 Subject: [PATCH 4/4] rerun