From d244cb912fe202f74d808d59d1e28024bceb83a1 Mon Sep 17 00:00:00 2001 From: Milly Date: Sun, 29 Dec 2024 01:15:20 +0900 Subject: [PATCH 1/2] test(async): use `delay` instead of `clearTimeout` to detect leaks --- async/abortable_test.ts | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/async/abortable_test.ts b/async/abortable_test.ts index a96839cd2df7..b52aff340664 100644 --- a/async/abortable_test.ts +++ b/async/abortable_test.ts @@ -1,47 +1,47 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. import { assertEquals, assertRejects } from "@std/assert"; import { abortable } from "./abortable.ts"; +import { delay } from "./delay.ts"; Deno.test("abortable() handles promise", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); + setTimeout(() => resolve("Hello"), 10); const result = await abortable(promise, c.signal); assertEquals(result, "Hello"); - clearTimeout(t); }); Deno.test("abortable() handles promise with aborted signal after delay", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); - setTimeout(() => c.abort(), 50); + setTimeout(() => resolve("Hello"), 10); + setTimeout(() => c.abort(), 5); const error = await assertRejects( () => abortable(promise, c.signal), DOMException, "The signal has been aborted", ); assertEquals(error.name, "AbortError"); - clearTimeout(t); + await delay(5); // wait for the promise to resolve }); Deno.test("abortable() handles promise with aborted signal after delay with reason", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); - setTimeout(() => c.abort(new Error("This is my reason")), 50); + setTimeout(() => resolve("Hello"), 10); + setTimeout(() => c.abort(new Error("This is my reason")), 5); await assertRejects( () => abortable(promise, c.signal), Error, "This is my reason", ); - clearTimeout(t); + await delay(5); // wait for the promise to resolve }); Deno.test("abortable() handles promise with already aborted signal", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); + setTimeout(() => resolve("Hello"), 10); c.abort(); const error = await assertRejects( async () => { @@ -51,26 +51,26 @@ Deno.test("abortable() handles promise with already aborted signal", async () => "The signal has been aborted", ); assertEquals(error.name, "AbortError"); - clearTimeout(t); + await delay(10); // wait for the promise to resolve }); Deno.test("abortable() handles promise with already aborted signal with reason", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); + setTimeout(() => resolve("Hello"), 10); c.abort(new Error("This is my reason")); await assertRejects( () => abortable(promise, c.signal), Error, "This is my reason", ); - clearTimeout(t); + await delay(10); // wait for the promise to resolve }); Deno.test("abortable.AsyncIterable()", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); + setTimeout(() => resolve("My promise resolved"), 10); const a = async function* () { yield "Hello"; await promise; @@ -78,19 +78,18 @@ Deno.test("abortable.AsyncIterable()", async () => { }; const items = await Array.fromAsync(abortable(a(), c.signal)); assertEquals(items, ["Hello", "World"]); - clearTimeout(t); }); Deno.test("abortable.AsyncIterable() handles aborted signal after delay", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); + setTimeout(() => resolve("My promise resolved"), 10); const a = async function* () { yield "Hello"; await promise; yield "World"; }; - setTimeout(() => c.abort(), 50); + setTimeout(() => c.abort(), 5); const items: string[] = []; const error = await assertRejects( async () => { @@ -103,13 +102,12 @@ Deno.test("abortable.AsyncIterable() handles aborted signal after delay", async ); assertEquals(error.name, "AbortError"); assertEquals(items, ["Hello"]); - clearTimeout(t); }); Deno.test("abortable.AsyncIterable() handles already aborted signal", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); - const t = setTimeout(() => resolve("Hello"), 100); + setTimeout(() => resolve("My promise resolved"), 10); const a = async function* () { yield "Hello"; await promise; @@ -128,7 +126,7 @@ Deno.test("abortable.AsyncIterable() handles already aborted signal", async () = ); assertEquals(error.name, "AbortError"); assertEquals(items, []); - clearTimeout(t); + await delay(10); // wait for the promise to resolve }); Deno.test("abortable.AsyncIterable() calls return before throwing", async () => { @@ -146,6 +144,7 @@ Deno.test("abortable.AsyncIterable() calls return before throwing", async () => }), return: () => { returnCalled = true; + clearTimeout(timeoutId); return Promise.resolve({ value: undefined, done: true }); }, }), @@ -164,7 +163,6 @@ Deno.test("abortable.AsyncIterable() calls return before throwing", async () => assertEquals(returnCalled, true); assertEquals(error.name, "AbortError"); assertEquals(items, []); - clearTimeout(timeoutId!); }); Deno.test("abortable.AsyncIterable() behaves just like original when not aborted", async () => { From 8b1850ac3e3fc2a70dcc77656d281e9b400300eb Mon Sep 17 00:00:00 2001 From: Milly Date: Sun, 29 Dec 2024 01:40:23 +0900 Subject: [PATCH 2/2] fix(async): `abortable` should prevent uncaught error when promise is rejected --- async/abortable.ts | 2 +- async/abortable_test.ts | 53 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/async/abortable.ts b/async/abortable.ts index 1b7a9268c3c7..b8f10f58ae2f 100644 --- a/async/abortable.ts +++ b/async/abortable.ts @@ -127,9 +127,9 @@ function abortablePromise( p: Promise, signal: AbortSignal, ): Promise { - if (signal.aborted) return Promise.reject(signal.reason); const { promise, reject } = Promise.withResolvers(); const abort = () => reject(signal.reason); + if (signal.aborted) abort(); signal.addEventListener("abort", abort, { once: true }); return Promise.race([promise, p]).finally(() => { signal.removeEventListener("abort", abort); diff --git a/async/abortable_test.ts b/async/abortable_test.ts index b52aff340664..d0d18838b2f6 100644 --- a/async/abortable_test.ts +++ b/async/abortable_test.ts @@ -3,7 +3,7 @@ import { assertEquals, assertRejects } from "@std/assert"; import { abortable } from "./abortable.ts"; import { delay } from "./delay.ts"; -Deno.test("abortable() handles promise", async () => { +Deno.test("abortable() handles resolved promise", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); setTimeout(() => resolve("Hello"), 10); @@ -11,7 +11,18 @@ Deno.test("abortable() handles promise", async () => { assertEquals(result, "Hello"); }); -Deno.test("abortable() handles promise with aborted signal after delay", async () => { +Deno.test("abortable() handles rejected promise", async () => { + const c = new AbortController(); + const { promise, reject } = Promise.withResolvers(); + setTimeout(() => reject(new Error("This is my error")), 10); + await assertRejects( + () => abortable(promise, c.signal), + Error, + "This is my error", + ); +}); + +Deno.test("abortable() handles resolved promise with aborted signal after delay", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); setTimeout(() => resolve("Hello"), 10); @@ -25,7 +36,22 @@ Deno.test("abortable() handles promise with aborted signal after delay", async ( await delay(5); // wait for the promise to resolve }); -Deno.test("abortable() handles promise with aborted signal after delay with reason", async () => { +Deno.test("abortable() handles rejected promise with aborted signal after delay", async () => { + const c = new AbortController(); + const { promise, reject } = Promise.withResolvers(); + setTimeout(() => reject(new Error("This is my error")), 10); + setTimeout(() => c.abort(), 5); + const error = await assertRejects( + () => abortable(promise, c.signal), + DOMException, + "The signal has been aborted", + ); + assertEquals(error.name, "AbortError"); + await delay(5); // wait for the promise to reject + // an uncaught error should not occur +}); + +Deno.test("abortable() handles resolved promise with aborted signal after delay with reason", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); setTimeout(() => resolve("Hello"), 10); @@ -38,7 +64,7 @@ Deno.test("abortable() handles promise with aborted signal after delay with reas await delay(5); // wait for the promise to resolve }); -Deno.test("abortable() handles promise with already aborted signal", async () => { +Deno.test("abortable() handles resolved promise with already aborted signal", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); setTimeout(() => resolve("Hello"), 10); @@ -54,7 +80,24 @@ Deno.test("abortable() handles promise with already aborted signal", async () => await delay(10); // wait for the promise to resolve }); -Deno.test("abortable() handles promise with already aborted signal with reason", async () => { +Deno.test("abortable() handles rejected promise with already aborted signal", async () => { + const c = new AbortController(); + const { promise, reject } = Promise.withResolvers(); + setTimeout(() => reject(new Error("This is my error")), 10); + c.abort(); + const error = await assertRejects( + async () => { + await abortable(promise, c.signal); + }, + DOMException, + "The signal has been aborted", + ); + assertEquals(error.name, "AbortError"); + await delay(10); // wait for the promise to reject + // an uncaught error should not occur +}); + +Deno.test("abortable() handles resolved promise with already aborted signal and reason", async () => { const c = new AbortController(); const { promise, resolve } = Promise.withResolvers(); setTimeout(() => resolve("Hello"), 10);