diff --git a/node/README.md b/node/README.md index 6b0d9258ed14..5f0d8511aeae 100644 --- a/node/README.md +++ b/node/README.md @@ -82,3 +82,56 @@ const cjsModule = require("./my_mod"); // Visits node_modules. const leftPad = require("left-pad"); ``` + +## Contributing + +When converting from promise-based to callback-based APIs, the most obvious way +is like this: + +```ts +promise.then((value) => callback(null, value)).catch(callback); +``` + +This has a subtle bug - if the callback throws an error, the catch statement +will also catch _that_ error, and the callback will be called twice. The correct +way to do it is like this: + +```ts +promise.then((value) => callback(null, value), callback); +``` + +The second parameter of `then` can also be used to catch errors, but only errors +from the existing promise, not the new one created by the callback. + +If the Deno equivalent is actually synchronous, there's a similar problem with +try/catch statements: + +```ts +try { + const value = process(); + callback(null, value); +} catch (err) { + callback(err); +} +``` + +Since the callback is called within the `try` block, any errors from it will be +caught and call the callback again. + +The correct way to do it is like this: + +```ts +let err, value; +try { + value = process(); +} catch (e) { + err = e; +} +if (err) { + callback(err); // Make sure arguments.length === 1 +} else { + callback(null, value); +} +``` + +It's not as clean, but prevents the callback being called twice. diff --git a/node/_crypto/pbkdf2.ts b/node/_crypto/pbkdf2.ts index 80aacdaf2059..2a63802e628a 100644 --- a/node/_crypto/pbkdf2.ts +++ b/node/_crypto/pbkdf2.ts @@ -162,19 +162,25 @@ export function pbkdf2( iterations: number, keylen: number, digest: Algorithms = "sha1", - callback: ((err?: Error, derivedKey?: Buffer) => void), + callback: ((err: Error | null, derivedKey?: Buffer) => void), ): void { - try { - const res = pbkdf2Sync( - password, - salt, - iterations, - keylen, - digest, - ); - - callback(undefined, res); - } catch (e) { - callback(e); - } + setTimeout(() => { + let err = null, res; + try { + res = pbkdf2Sync( + password, + salt, + iterations, + keylen, + digest, + ); + } catch (e) { + err = e; + } + if (err) { + callback(err); + } else { + callback(null, res); + } + }, 0); } diff --git a/node/_crypto/pbkdf2_test.ts b/node/_crypto/pbkdf2_test.ts index 798805ce4413..29f149cbe87e 100644 --- a/node/_crypto/pbkdf2_test.ts +++ b/node/_crypto/pbkdf2_test.ts @@ -3,7 +3,12 @@ import { pbkdf2, pbkdf2Sync, } from "./pbkdf2.ts"; -import { assert, assertEquals } from "../../testing/asserts.ts"; +import { + assert, + assertEquals, + assertStringIncludes, +} from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; type Pbkdf2Fixture = { key: string | Float64Array | Int32Array | Uint8Array; @@ -412,3 +417,11 @@ Deno.test("pbkdf2Sync hashes data correctly", () => { } }); }); + +Deno.test("[std/node/crypto] pbkdf2 callback isn't called twice if error is thrown", async () => { + const importUrl = new URL("./pbkdf2.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { pbkdf2 } from ${JSON.stringify(importUrl)}`, + invocation: 'pbkdf2("password", "salt", 1, 32, "sha1", ', + }); +}); diff --git a/node/_crypto/randomBytes.ts b/node/_crypto/randomBytes.ts index 28c7e454eeee..335c92d6829b 100644 --- a/node/_crypto/randomBytes.ts +++ b/node/_crypto/randomBytes.ts @@ -39,8 +39,9 @@ export default function randomBytes( cb?: (err: Error | null, buf?: Buffer) => void, ): Buffer | void { if (typeof cb === "function") { + let err: Error | null = null, bytes: Buffer; try { - cb(null, generateRandomBytes(size)); + bytes = generateRandomBytes(size); } catch (e) { //NodeJS nonsense //If the size is out of range it will throw sync, otherwise throw async @@ -50,9 +51,16 @@ export default function randomBytes( ) { throw e; } else { - cb(e); + err = e; } } + setTimeout(() => { + if (err) { + cb(err); + } else { + cb(null, bytes); + } + }, 0); } else { return generateRandomBytes(size); } diff --git a/node/_crypto/randomBytes_test.ts b/node/_crypto/randomBytes_test.ts index 701127dce2e9..6dd2091e15f3 100644 --- a/node/_crypto/randomBytes_test.ts +++ b/node/_crypto/randomBytes_test.ts @@ -1,4 +1,11 @@ -import { assert, assertEquals, assertThrows } from "../../testing/asserts.ts"; +import { + assert, + assertEquals, + assertStringIncludes, + assertThrows, + assertThrowsAsync, +} from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import randomBytes, { MAX_RANDOM_VALUES, MAX_SIZE } from "./randomBytes.ts"; Deno.test("randomBytes sync works correctly", function () { @@ -59,10 +66,24 @@ Deno.test("randomBytes async works correctly", function () { assert(!err); }) ); - assertThrows(() => - randomBytes(-1, function (err) { - //Shouldn't throw async - assert(!err); + assertThrowsAsync(() => + new Promise((resolve, reject) => { + randomBytes(-1, function (err, res) { + //Shouldn't throw async + if (err) { + reject(err); + } else { + resolve(res); + } + }); }) ); }); + +Deno.test("[std/node/crypto] randomBytes callback isn't called twice if error is thrown", async () => { + const importUrl = new URL("./randomBytes.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import randomBytes from ${JSON.stringify(importUrl)}`, + invocation: "randomBytes(0, ", + }); +}); diff --git a/node/_fs/_fs_appendFile.ts b/node/_fs/_fs_appendFile.ts index fab72d0f7328..ee01b9cc8c75 100644 --- a/node/_fs/_fs_appendFile.ts +++ b/node/_fs/_fs_appendFile.ts @@ -35,7 +35,7 @@ export function appendFile( new Promise((resolve, reject) => { if (typeof pathOrRid === "number") { rid = pathOrRid; - Deno.write(rid, buffer).then(resolve).catch(reject); + Deno.write(rid, buffer).then(resolve, reject); } else { const mode: number | undefined = isFileOptions(options) ? options.mode @@ -53,15 +53,13 @@ export function appendFile( rid = openedFileRid; return Deno.write(openedFileRid, buffer); }) - .then(resolve) - .catch(reject); + .then(resolve, reject); } }) .then(() => { closeRidIfNecessary(typeof pathOrRid === "string", rid); - callbackFn(); - }) - .catch((err) => { + callbackFn(null); + }, (err) => { closeRidIfNecessary(typeof pathOrRid === "string", rid); callbackFn(err); }); diff --git a/node/_fs/_fs_appendFile_test.ts b/node/_fs/_fs_appendFile_test.ts index af430ec97fd8..6a77974fd816 100644 --- a/node/_fs/_fs_appendFile_test.ts +++ b/node/_fs/_fs_appendFile_test.ts @@ -2,6 +2,7 @@ import { assertEquals, assertThrows, fail } from "../../testing/asserts.ts"; import { appendFile, appendFileSync } from "./_fs_appendFile.ts"; import { fromFileUrl } from "../path.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; const decoder = new TextDecoder("utf-8"); @@ -78,8 +79,7 @@ Deno.test({ .then(async () => { const data = await Deno.readFile(tempFile); assertEquals(decoder.decode(data), "hello world"); - }) - .catch(() => { + }, () => { fail("No error expected"); }) .finally(async () => { @@ -103,8 +103,7 @@ Deno.test({ assertEquals(Deno.resources(), openResourcesBeforeAppend); const data = await Deno.readFile("_fs_appendFile_test_file.txt"); assertEquals(decoder.decode(data), "hello world"); - }) - .catch((err) => { + }, (err) => { fail("No error was expected: " + err); }) .finally(async () => { @@ -128,8 +127,7 @@ Deno.test({ assertEquals(Deno.resources(), openResourcesBeforeAppend); const data = await Deno.readFile(fromFileUrl(fileURL)); assertEquals(decoder.decode(data), "hello world"); - }) - .catch((err) => { + }, (err) => { fail("No error was expected: " + err); }) .finally(async () => { @@ -152,8 +150,7 @@ Deno.test({ }) .then(() => { fail("Expected error to be thrown"); - }) - .catch(() => { + }, () => { assertEquals(Deno.resources(), openResourcesBeforeAppend); }) .finally(async () => { @@ -235,8 +232,7 @@ Deno.test({ assertEquals(Deno.resources(), openResourcesBeforeAppend); const data = await Deno.readFile("_fs_appendFile_test_file.txt"); assertEquals(data, testData); - }) - .catch((err) => { + }, (err) => { fail("No error was expected: " + err); }) .finally(async () => { @@ -244,3 +240,15 @@ Deno.test({ }); }, }); + +Deno.test("[std/node/fs] appendFile callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_appendFile.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { appendFile } from ${JSON.stringify(importUrl)}`, + invocation: `appendFile(${JSON.stringify(tempFile)}, "hello world", `, + async cleanup() { + await Deno.remove(tempFile); + }, + }); +}); diff --git a/node/_fs/_fs_chmod.ts b/node/_fs/_fs_chmod.ts index cb673da674bd..bf4031d2ccc6 100644 --- a/node/_fs/_fs_chmod.ts +++ b/node/_fs/_fs_chmod.ts @@ -15,9 +15,7 @@ export function chmod( ): void { path = path instanceof URL ? fromFileUrl(path) : path; - Deno.chmod(path, getResolvedMode(mode)) - .then(() => callback()) - .catch(callback); + Deno.chmod(path, getResolvedMode(mode)).then(() => callback(null), callback); } /** diff --git a/node/_fs/_fs_chmod_test.ts b/node/_fs/_fs_chmod_test.ts index 5f946794e11b..4d8647782e78 100644 --- a/node/_fs/_fs_chmod_test.ts +++ b/node/_fs/_fs_chmod_test.ts @@ -1,5 +1,6 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. import { assert, fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { chmod, chmodSync } from "./_fs_chmod.ts"; Deno.test({ @@ -18,8 +19,7 @@ Deno.test({ const newFileMode: number | null = Deno.lstatSync(tempFile).mode; assert(newFileMode && originalFileMode); assert(newFileMode === 33279 && newFileMode > originalFileMode); - }) - .catch(() => { + }, () => { fail(); }) .finally(() => { @@ -42,3 +42,19 @@ Deno.test({ Deno.removeSync(tempFile); }, }); + +Deno.test({ + name: "[std/node/fs] chmod callback isn't called twice if error is thrown", + ignore: Deno.build.os === "windows", + async fn() { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_chmod.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { chmod } from ${JSON.stringify(importUrl)}`, + invocation: `chmod(${JSON.stringify(tempFile)}, 0o777, `, + async cleanup() { + await Deno.remove(tempFile); + }, + }); + }, +}); diff --git a/node/_fs/_fs_chown.ts b/node/_fs/_fs_chown.ts index ca709f2ea917..f9e7326cec16 100644 --- a/node/_fs/_fs_chown.ts +++ b/node/_fs/_fs_chown.ts @@ -14,9 +14,7 @@ export function chown( ): void { path = path instanceof URL ? fromFileUrl(path) : path; - Deno.chown(path, uid, gid) - .then(() => callback()) - .catch(callback); + Deno.chown(path, uid, gid).then(() => callback(null), callback); } /** diff --git a/node/_fs/_fs_chown_test.ts b/node/_fs/_fs_chown_test.ts index dc881c27ab54..8d74f657a03f 100644 --- a/node/_fs/_fs_chown_test.ts +++ b/node/_fs/_fs_chown_test.ts @@ -1,5 +1,6 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. import { assertEquals, fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { chown, chownSync } from "./_fs_chown.ts"; // chown is difficult to test. Best we can do is set the existing user id/group @@ -24,8 +25,7 @@ Deno.test({ const newGroupId: number | null = Deno.lstatSync(tempFile).gid; assertEquals(newUserId, originalUserId); assertEquals(newGroupId, originalGroupId); - }) - .catch(() => { + }, () => { fail(); }) .finally(() => { @@ -50,3 +50,20 @@ Deno.test({ Deno.removeSync(tempFile); }, }); + +Deno.test({ + name: "[std/node/fs] chown callback isn't called twice if error is thrown", + ignore: Deno.build.os === "windows", + async fn() { + const tempFile = await Deno.makeTempFile(); + const { uid, gid } = await Deno.lstat(tempFile); + const importUrl = new URL("./_fs_chown.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { chown } from ${JSON.stringify(importUrl)}`, + invocation: `chown(${JSON.stringify(tempFile)}, ${uid}, ${gid}, `, + async cleanup() { + await Deno.remove(tempFile); + }, + }); + }, +}); diff --git a/node/_fs/_fs_close.ts b/node/_fs/_fs_close.ts index 8e9a1f62030f..721827421902 100644 --- a/node/_fs/_fs_close.ts +++ b/node/_fs/_fs_close.ts @@ -2,14 +2,15 @@ import type { CallbackWithError } from "./_fs_common.ts"; export function close(fd: number, callback: CallbackWithError): void { - queueMicrotask(() => { + setTimeout(() => { + let error = null; try { Deno.close(fd); - callback(null); } catch (err) { - callback(err); + error = err; } - }); + callback(error); + }, 0); } export function closeSync(fd: number): void { diff --git a/node/_fs/_fs_close_test.ts b/node/_fs/_fs_close_test.ts index 047f6d977732..df5a932409ef 100644 --- a/node/_fs/_fs_close_test.ts +++ b/node/_fs/_fs_close_test.ts @@ -1,5 +1,6 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. import { assert, assertThrows, fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { close, closeSync } from "./_fs_close.ts"; Deno.test({ @@ -17,8 +18,7 @@ Deno.test({ }) .then(() => { assert(!Deno.resources()[file.rid]); - }) - .catch(() => { + }, () => { fail("No error expected"); }) .finally(async () => { @@ -78,3 +78,19 @@ Deno.test({ assertThrows(() => closeSync(-1)); }, }); + +Deno.test("[std/node/fs] close callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_close.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: ` + import { close } from ${JSON.stringify(importUrl)}; + + const file = await Deno.open(${JSON.stringify(tempFile)}); + `, + invocation: "close(file.rid, ", + async cleanup() { + await Deno.remove(tempFile); + }, + }); +}); diff --git a/node/_fs/_fs_common.ts b/node/_fs/_fs_common.ts index 73a0a81ef8dc..c233b2d76b2f 100644 --- a/node/_fs/_fs_common.ts +++ b/node/_fs/_fs_common.ts @@ -6,7 +6,7 @@ import { TextEncodings, } from "../_utils.ts"; -export type CallbackWithError = (err?: Error | null) => void; +export type CallbackWithError = (err: Error | null) => void; export interface FileOptions { encoding?: Encodings; diff --git a/node/_fs/_fs_copy.ts b/node/_fs/_fs_copy.ts index 00d17798fbc3..3242b97b3014 100644 --- a/node/_fs/_fs_copy.ts +++ b/node/_fs/_fs_copy.ts @@ -9,9 +9,7 @@ export function copyFile( ): void { source = source instanceof URL ? fromFileUrl(source) : source; - Deno.copyFile(source, destination) - .then(() => callback()) - .catch(callback); + Deno.copyFile(source, destination).then(() => callback(null), callback); } export function copyFileSync(source: string | URL, destination: string): void { diff --git a/node/_fs/_fs_copy_test.ts b/node/_fs/_fs_copy_test.ts index c61fffb14fec..4fdf78cb0d97 100644 --- a/node/_fs/_fs_copy_test.ts +++ b/node/_fs/_fs_copy_test.ts @@ -1,5 +1,7 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. +import * as path from "../../path/mod.ts"; import { assert } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { copyFile, copyFileSync } from "./_fs_copy.ts"; import { existsSync } from "./_fs_exists.ts"; @@ -29,3 +31,23 @@ Deno.test({ Deno.removeSync(destFile); }, }); + +Deno.test("[std/node/fs] copyFile callback isn't called twice if error is thrown", async () => { + // The correct behaviour is not to catch any errors thrown, + // but that means there'll be an uncaught error and the test will fail. + // So the only way to test this is to spawn a subprocess, and succeed if it has a non-zero exit code. + // (assertThrowsAsync won't work because there's no way to catch the error.) + const tempDir = await Deno.makeTempDir(); + const tempFile1 = path.join(tempDir, "file1.txt"); + const tempFile2 = path.join(tempDir, "file2.txt"); + await Deno.writeTextFile(tempFile1, "hello world"); + const importUrl = new URL("./_fs_copy.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { copyFile } from ${JSON.stringify(importUrl)}`, + invocation: `copyFile(${JSON.stringify(tempFile1)}, + ${JSON.stringify(tempFile2)}, `, + async cleanup() { + await Deno.remove(tempDir, { recursive: true }); + }, + }); +}); diff --git a/node/_fs/_fs_dir.ts b/node/_fs/_fs_dir.ts index b2353146be87..4f7beec31fff 100644 --- a/node/_fs/_fs_dir.ts +++ b/node/_fs/_fs_dir.ts @@ -32,10 +32,9 @@ export default class Dir { if (callback) { callback(null, value ? value : null); } - }) - .catch((err) => { + }, (err) => { if (callback) { - callback(err, null); + callback(err); } reject(err); }); @@ -59,18 +58,11 @@ export default class Dir { */ // deno-lint-ignore no-explicit-any close(callback?: (...args: any[]) => void): Promise { - return new Promise((resolve, reject) => { - try { - if (callback) { - callback(null); - } - resolve(); - } catch (err) { - if (callback) { - callback(err); - } - reject(err); + return new Promise((resolve) => { + if (callback) { + callback(null); } + resolve(); }); } diff --git a/node/_fs/_fs_dir_test.ts b/node/_fs/_fs_dir_test.ts index b2b0998b3628..f12563fd51db 100644 --- a/node/_fs/_fs_dir_test.ts +++ b/node/_fs/_fs_dir_test.ts @@ -1,5 +1,6 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. import { assert, assertEquals, fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import Dir from "./_fs_dir.ts"; import type Dirent from "./_fs_dirent.ts"; @@ -165,3 +166,35 @@ Deno.test({ } }, }); + +Deno.test("[std/node/fs] Dir.close callback isn't called twice if error is thrown", async () => { + const tempDir = await Deno.makeTempDir(); + const importUrl = new URL("./_fs_dir.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: ` + import Dir from ${JSON.stringify(importUrl)}; + + const dir = new Dir(${JSON.stringify(tempDir)}); + `, + invocation: "dir.close(", + async cleanup() { + await Deno.remove(tempDir); + }, + }); +}); + +Deno.test("[std/node/fs] Dir.read callback isn't called twice if error is thrown", async () => { + const tempDir = await Deno.makeTempDir(); + const importUrl = new URL("./_fs_dir.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: ` + import Dir from ${JSON.stringify(importUrl)}; + + const dir = new Dir(${JSON.stringify(tempDir)}); + `, + invocation: "dir.read(", + async cleanup() { + await Deno.remove(tempDir); + }, + }); +}); diff --git a/node/_fs/_fs_exists.ts b/node/_fs/_fs_exists.ts index 91f2ce10596d..5cfa26a9d119 100644 --- a/node/_fs/_fs_exists.ts +++ b/node/_fs/_fs_exists.ts @@ -10,11 +10,7 @@ type ExitsCallback = (exists: boolean) => void; */ export function exists(path: string | URL, callback: ExitsCallback): void { path = path instanceof URL ? fromFileUrl(path) : path; - Deno.lstat(path) - .then(() => { - callback(true); - }) - .catch(() => callback(false)); + Deno.lstat(path).then(() => callback(true), () => callback(false)); } /** diff --git a/node/_fs/_fs_exists_test.ts b/node/_fs/_fs_exists_test.ts index 6e120af9827d..ab0ae5c15a33 100644 --- a/node/_fs/_fs_exists_test.ts +++ b/node/_fs/_fs_exists_test.ts @@ -1,5 +1,9 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. -import { assertEquals } from "../../testing/asserts.ts"; +import { + assert, + assertEquals, + assertStringIncludes, +} from "../../testing/asserts.ts"; import { exists, existsSync } from "./_fs_exists.ts"; Deno.test("existsFile", async function () { @@ -23,3 +27,32 @@ Deno.test("existsSyncFile", function () { Deno.removeSync(tmpFilePath); assertEquals(existsSync("./notAvailable.txt"), false); }); + +Deno.test("[std/node/fs] exists callback isn't called twice if error is thrown", async () => { + // This doesn't use `assertCallbackErrorUncaught()` because `exists()` doesn't return a standard node callback, which is what it expects. + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_exists.ts", import.meta.url); + const p = Deno.run({ + cmd: [ + Deno.execPath(), + "eval", + "--no-check", + ` + import { exists } from ${JSON.stringify(importUrl)}; + + exists(${JSON.stringify(tempFile)}, (exists) => { + // If the bug is present and the callback is called again with false (meaning an error occured), + // don't throw another error, so if the subprocess fails we know it had the correct behaviour. + if (exists) throw new Error("success"); + });`, + ], + stderr: "piped", + }); + const status = await p.status(); + const stderr = new TextDecoder().decode(await Deno.readAll(p.stderr)); + p.close(); + p.stderr.close(); + await Deno.remove(tempFile); + assert(!status.success); + assertStringIncludes(stderr, "Error: success"); +}); diff --git a/node/_fs/_fs_link.ts b/node/_fs/_fs_link.ts index 0fe7e1b4aac3..14a8189cb542 100644 --- a/node/_fs/_fs_link.ts +++ b/node/_fs/_fs_link.ts @@ -16,9 +16,7 @@ export function link( : existingPath; newPath = newPath instanceof URL ? fromFileUrl(newPath) : newPath; - Deno.link(existingPath, newPath) - .then(() => callback()) - .catch(callback); + Deno.link(existingPath, newPath).then(() => callback(null), callback); } /** diff --git a/node/_fs/_fs_link_test.ts b/node/_fs/_fs_link_test.ts index 4804b2eed7c1..9e2aeb8b6a4a 100644 --- a/node/_fs/_fs_link_test.ts +++ b/node/_fs/_fs_link_test.ts @@ -1,7 +1,8 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. -import { assertEquals, fail } from "../../testing/asserts.ts"; +import * as path from "../../path/mod.ts"; +import { assert, assertEquals, fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { link, linkSync } from "./_fs_link.ts"; -import { assert } from "../../testing/asserts.ts"; Deno.test({ name: "ASYNC: hard linking files works as expected", @@ -16,8 +17,7 @@ Deno.test({ }) .then(() => { assertEquals(Deno.statSync(tempFile), Deno.statSync(linkedFile)); - }) - .catch(() => { + }, () => { fail("Expected to succeed"); }) .finally(() => { @@ -39,8 +39,7 @@ Deno.test({ }) .then(() => { fail("Expected to succeed"); - }) - .catch((err) => { + }, (err) => { assert(err); failed = true; }); @@ -60,3 +59,19 @@ Deno.test({ Deno.removeSync(linkedFile); }, }); + +Deno.test("[std/node/fs] link callback isn't called twice if error is thrown", async () => { + const tempDir = await Deno.makeTempDir(); + const tempFile = path.join(tempDir, "file.txt"); + const linkFile = path.join(tempDir, "link.txt"); + await Deno.writeTextFile(tempFile, "hello world"); + const importUrl = new URL("./_fs_link.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { link } from ${JSON.stringify(importUrl)}`, + invocation: `link(${JSON.stringify(tempFile)}, + ${JSON.stringify(linkFile)}, `, + async cleanup() { + await Deno.remove(tempDir, { recursive: true }); + }, + }); +}); diff --git a/node/_fs/_fs_lstat.ts b/node/_fs/_fs_lstat.ts index 0b79fb665d31..55624cd5eafd 100644 --- a/node/_fs/_fs_lstat.ts +++ b/node/_fs/_fs_lstat.ts @@ -27,8 +27,7 @@ export function lstat( (typeof optionsOrCallback === "function" ? optionsOrCallback : maybeCallback) as ( - err: Error | undefined, - stat: BigIntStats | Stats, + ...args: [Error] | [null, BigIntStats | Stats] ) => void; const options = typeof optionsOrCallback === "object" ? optionsOrCallback @@ -36,9 +35,10 @@ export function lstat( if (!callback) throw new Error("No callback function supplied"); - Deno.lstat(path) - .then((stat) => callback(undefined, CFISBIS(stat, options.bigint))) - .catch((err) => callback(err, err)); + Deno.lstat(path).then( + (stat) => callback(null, CFISBIS(stat, options.bigint)), + (err) => callback(err), + ); } export function lstatSync(path: string | URL): Stats; diff --git a/node/_fs/_fs_lstat_test.ts b/node/_fs/_fs_lstat_test.ts index 1da0a562f48d..b8c562538350 100644 --- a/node/_fs/_fs_lstat_test.ts +++ b/node/_fs/_fs_lstat_test.ts @@ -1,5 +1,6 @@ import { lstat, lstatSync } from "./_fs_lstat.ts"; import { fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { assertStats, assertStatsBigInt } from "./_fs_stat_test.ts"; import type { BigIntStats, Stats } from "./_fs_stat.ts"; @@ -15,8 +16,7 @@ Deno.test({ }) .then((stat) => { assertStats(stat, Deno.lstatSync(file)); - }) - .catch(() => fail()) + }, () => fail()) .finally(() => { Deno.removeSync(file); }); @@ -41,8 +41,10 @@ Deno.test({ resolve(stat); }); }) - .then((stat) => assertStatsBigInt(stat, Deno.lstatSync(file))) - .catch(() => fail()) + .then( + (stat) => assertStatsBigInt(stat, Deno.lstatSync(file)), + () => fail(), + ) .finally(() => Deno.removeSync(file)); }, }); @@ -54,3 +56,15 @@ Deno.test({ assertStatsBigInt(lstatSync(file, { bigint: true }), Deno.lstatSync(file)); }, }); + +Deno.test("[std/node/fs] lstat callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_lstat.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { lstat } from ${JSON.stringify(importUrl)}`, + invocation: `lstat(${JSON.stringify(tempFile)}, `, + async cleanup() { + await Deno.remove(tempFile); + }, + }); +}); diff --git a/node/_fs/_fs_mkdir.ts b/node/_fs/_fs_mkdir.ts index 3a060ee76ab2..94cc24a42fc3 100644 --- a/node/_fs/_fs_mkdir.ts +++ b/node/_fs/_fs_mkdir.ts @@ -39,10 +39,9 @@ export function mkdir( Deno.mkdir(path, { recursive, mode }) .then(() => { if (typeof callback === "function") { - callback(); + callback(null); } - }) - .catch((err) => { + }, (err) => { if (typeof callback === "function") { callback(err); } diff --git a/node/_fs/_fs_mkdir_test.ts b/node/_fs/_fs_mkdir_test.ts index e3d68ea9cf29..c13d065fe8de 100644 --- a/node/_fs/_fs_mkdir_test.ts +++ b/node/_fs/_fs_mkdir_test.ts @@ -1,5 +1,7 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. +import * as path from "../../path/mod.ts"; import { assert } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { mkdir, mkdirSync } from "./_fs_mkdir.ts"; import { existsSync } from "./_fs_exists.ts"; @@ -27,3 +29,16 @@ Deno.test({ Deno.removeSync(tmpDir); }, }); + +Deno.test("[std/node/fs] mkdir callback isn't called twice if error is thrown", async () => { + const tempDir = await Deno.makeTempDir(); + const subdir = path.join(tempDir, "subdir"); + const importUrl = new URL("./_fs_mkdir.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { mkdir } from ${JSON.stringify(importUrl)}`, + invocation: `mkdir(${JSON.stringify(subdir)}, `, + async cleanup() { + await Deno.remove(tempDir, { recursive: true }); + }, + }); +}); diff --git a/node/_fs/_fs_mkdtemp.ts b/node/_fs/_fs_mkdtemp.ts index a249f1911224..226399aa4dea 100644 --- a/node/_fs/_fs_mkdtemp.ts +++ b/node/_fs/_fs_mkdtemp.ts @@ -7,7 +7,7 @@ import { } from "../_errors.ts"; export type mkdtempCallback = ( - err: Error | undefined, + err: Error | null, directory?: string, ) => void; @@ -35,7 +35,7 @@ export function mkdtemp( { recursive: false, mode: 0o700 }, (err: Error | null | undefined) => { if (err) callback(err); - else callback(undefined, decode(path, encoding)); + else callback(null, decode(path, encoding)); }, ); } diff --git a/node/_fs/_fs_open.ts b/node/_fs/_fs_open.ts index bf53115de6c9..55ecbdc1eb29 100644 --- a/node/_fs/_fs_open.ts +++ b/node/_fs/_fs_open.ts @@ -17,7 +17,7 @@ type openFlags = | "w+" | "wx+"; -type openCallback = (err: Error | undefined, fd: number) => void; +type openCallback = (err: Error | null, fd: number) => void; function convertFlagAndModeToOptions( flag?: openFlags, @@ -61,20 +61,26 @@ export function open( if (["ax", "ax+", "wx", "wx+"].includes(flags || "") && existsSync(path)) { const err = new Error(`EEXIST: file already exists, open '${path}'`); - callback(err, 0); + (callback as (err: Error) => void)(err); } else { if (flags === "as" || flags === "as+") { + let err: Error | null = null, res: number; try { - const res = openSync(path, flags, mode); - callback(undefined, res); + res = openSync(path, flags, mode); } catch (error) { - callback(error, error); + err = error; + } + if (err) { + (callback as (err: Error) => void)(err); + } else { + callback(null, res!); } return; } - Deno.open(path, convertFlagAndModeToOptions(flags, mode)) - .then((file) => callback(undefined, file.rid)) - .catch((err) => callback(err, err)); + Deno.open(path, convertFlagAndModeToOptions(flags, mode)).then( + (file) => callback(null, file.rid), + (err) => (callback as (err: Error) => void)(err), + ); } } diff --git a/node/_fs/_fs_open_test.ts b/node/_fs/_fs_open_test.ts index 16f60110e527..f32a8ce0a7c7 100644 --- a/node/_fs/_fs_open_test.ts +++ b/node/_fs/_fs_open_test.ts @@ -4,6 +4,7 @@ import { assertThrows, fail, } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { open, openSync } from "./_fs_open.ts"; import { join, parse } from "../../path/mod.ts"; import { existsSync } from "../../fs/mod.ts"; @@ -25,8 +26,7 @@ Deno.test({ .then((fd) => { fd1 = fd; assert(Deno.resources()[fd], `${fd}`); - }) - .catch(() => fail()) + }, () => fail()) .finally(() => closeSync(fd1)); }, }); @@ -207,3 +207,15 @@ Deno.test({ Deno.removeSync(file); }, }); + +Deno.test("[std/node/fs] open callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_open.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { open } from ${JSON.stringify(importUrl)}`, + invocation: `open(${JSON.stringify(tempFile)}, `, + async cleanup() { + await Deno.remove(tempFile); + }, + }); +}); diff --git a/node/_fs/_fs_readFile.ts b/node/_fs/_fs_readFile.ts index 7540c8207ebb..4ad763e25c37 100644 --- a/node/_fs/_fs_readFile.ts +++ b/node/_fs/_fs_readFile.ts @@ -69,7 +69,7 @@ export function readFile( } const buffer = maybeDecode(data, encoding); (cb as BinaryCallback)(null, buffer); - }).catch((err) => cb && cb(err)); + }, (err) => cb && cb(err)); } } diff --git a/node/_fs/_fs_readFile_test.ts b/node/_fs/_fs_readFile_test.ts index 50fe569ff73e..7af32c8a13b6 100644 --- a/node/_fs/_fs_readFile_test.ts +++ b/node/_fs/_fs_readFile_test.ts @@ -1,4 +1,5 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { readFile, readFileSync } from "./_fs_readFile.ts"; import * as path from "../../path/mod.ts"; import { assert, assertEquals } from "../../testing/asserts.ts"; @@ -103,3 +104,15 @@ Deno.test("readFileEncodeAsString", function () { assertEquals(typeof data, "string"); assertEquals(data as string, "hello world"); }); + +Deno.test("[std/node/fs] readFile callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_readFile.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { readFile } from ${JSON.stringify(importUrl)}`, + invocation: `readFile(${JSON.stringify(tempFile)}, `, + async cleanup() { + await Deno.remove(tempFile); + }, + }); +}); diff --git a/node/_fs/_fs_readdir.ts b/node/_fs/_fs_readdir.ts index 9034eccf8202..e36bf5ecf94c 100644 --- a/node/_fs/_fs_readdir.ts +++ b/node/_fs/_fs_readdir.ts @@ -11,13 +11,12 @@ type readDirOptions = { withFileTypes?: boolean; }; -type readDirCallback = (err: Error | undefined, files: string[]) => void; +type readDirCallback = (err: Error | null, files: string[]) => void; -type readDirCallbackDirent = (err: Error | undefined, files: Dirent[]) => void; +type readDirCallbackDirent = (err: Error | null, files: Dirent[]) => void; type readDirBoth = ( - err: Error | undefined, - files: string[] | Dirent[] | Array, + ...args: [Error] | [null, string[] | Dirent[] | Array] ) => void; export function readdir( @@ -62,7 +61,7 @@ export function readdir( asyncIterableToCallback(Deno.readDir(path), (val, done) => { if (typeof path !== "string") return; if (done) { - callback(undefined, result); + callback(null, result); return; } if (options?.withFileTypes) { @@ -70,7 +69,7 @@ export function readdir( } else result.push(decode(val.name)); }); } catch (error) { - callback(error, result); + callback(error); } } diff --git a/node/_fs/_fs_readdir_test.ts b/node/_fs/_fs_readdir_test.ts index 169e102455b3..165cb81410a0 100644 --- a/node/_fs/_fs_readdir_test.ts +++ b/node/_fs/_fs_readdir_test.ts @@ -1,4 +1,5 @@ import { assertEquals, assertNotEquals, fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { readdir, readdirSync } from "./_fs_readdir.ts"; import { join } from "../../path/mod.ts"; @@ -12,8 +13,7 @@ Deno.test({ resolve(files); }); }) - .then((files) => assertEquals(files, [])) - .catch(() => fail()) + .then((files) => assertEquals(files, []), () => fail()) .finally(() => Deno.removeSync(dir)); }, }); @@ -40,10 +40,14 @@ Deno.test({ resolve(files); }); }) - .then((files) => - assertEqualsArrayAnyOrder(files, ["file1.txt", "some_dir", "file2.txt"]) + .then( + (files) => + assertEqualsArrayAnyOrder( + files, + ["file1.txt", "some_dir", "file2.txt"], + ), + () => fail(), ) - .catch(() => fail()) .finally(() => Deno.removeSync(dir, { recursive: true })); }, }); @@ -69,3 +73,19 @@ Deno.test({ ); }, }); + +Deno.test("[std/node/fs] readdir callback isn't called twice if error is thrown", async () => { + // The correct behaviour is not to catch any errors thrown, + // but that means there'll be an uncaught error and the test will fail. + // So the only way to test this is to spawn a subprocess, and succeed if it has a non-zero exit code. + // (assertThrowsAsync won't work because there's no way to catch the error.) + const tempDir = await Deno.makeTempDir(); + const importUrl = new URL("./_fs_readdir.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { readdir } from ${JSON.stringify(importUrl)}`, + invocation: `readdir(${JSON.stringify(tempDir)}, `, + async cleanup() { + await Deno.remove(tempDir); + }, + }); +}); diff --git a/node/_fs/_fs_readlink_test.ts b/node/_fs/_fs_readlink_test.ts index 06cd7e2dcc36..cb3dd25b4265 100644 --- a/node/_fs/_fs_readlink_test.ts +++ b/node/_fs/_fs_readlink_test.ts @@ -1,4 +1,5 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { readlink, readlinkSync } from "./_fs_readlink.ts"; import { assert, assertEquals } from "../../testing/asserts.ts"; import * as path from "../path.ts"; @@ -64,3 +65,11 @@ Deno.test({ assertEquals(new TextDecoder().decode(data as Uint8Array), oldname); }, }); + +Deno.test("[std/node/fs] readlink callback isn't called twice if error is thrown", async () => { + const importUrl = new URL("./_fs_readlink.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { readlink } from ${JSON.stringify(importUrl)}`, + invocation: `readlink(${JSON.stringify(newname)}, `, + }); +}); diff --git a/node/_fs/_fs_realpath.ts b/node/_fs/_fs_realpath.ts index 586a87cfc06d..95e699d65569 100644 --- a/node/_fs/_fs_realpath.ts +++ b/node/_fs/_fs_realpath.ts @@ -12,9 +12,10 @@ export function realpath( if (!callback) { throw new Error("No callback function supplied"); } - Deno.realPath(path) - .then((path) => callback!(null, path)) - .catch((err) => callback!(err)); + Deno.realPath(path).then( + (path) => callback!(null, path), + (err) => callback!(err), + ); } export function realpathSync(path: string): string { diff --git a/node/_fs/_fs_realpath_test.ts b/node/_fs/_fs_realpath_test.ts index 9ab0173c157f..08eb3ef16096 100644 --- a/node/_fs/_fs_realpath_test.ts +++ b/node/_fs/_fs_realpath_test.ts @@ -1,4 +1,6 @@ +import * as path from "../../path/mod.ts"; import { assertEquals } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { realpath, realpathSync } from "./_fs_realpath.ts"; Deno.test("realpath", async function () { @@ -34,3 +36,19 @@ Deno.test("realpathSync", function () { const realSymLinkPath = realpathSync(tempFileAlias); assertEquals(realPath, realSymLinkPath); }); + +Deno.test("[std/node/fs] realpath callback isn't called twice if error is thrown", async () => { + const tempDir = await Deno.makeTempDir(); + const tempFile = path.join(tempDir, "file.txt"); + const linkFile = path.join(tempDir, "link.txt"); + await Deno.writeTextFile(tempFile, "hello world"); + await Deno.symlink(tempFile, linkFile, { type: "file" }); + const importUrl = new URL("./_fs_realpath.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { realpath } from ${JSON.stringify(importUrl)}`, + invocation: `realpath(${JSON.stringify(`${tempDir}/link.txt`)}, `, + async cleanup() { + await Deno.remove(tempDir, { recursive: true }); + }, + }); +}); diff --git a/node/_fs/_fs_rename.ts b/node/_fs/_fs_rename.ts index ee7c00977074..b121eacf9c9a 100644 --- a/node/_fs/_fs_rename.ts +++ b/node/_fs/_fs_rename.ts @@ -10,9 +10,7 @@ export function rename( if (!callback) throw new Error("No callback function supplied"); - Deno.rename(oldPath, newPath) - .then((_) => callback()) - .catch(callback); + Deno.rename(oldPath, newPath).then((_) => callback(), callback); } export function renameSync(oldPath: string | URL, newPath: string | URL) { diff --git a/node/_fs/_fs_rename_test.ts b/node/_fs/_fs_rename_test.ts index d0ba896446c5..e35e5282e708 100644 --- a/node/_fs/_fs_rename_test.ts +++ b/node/_fs/_fs_rename_test.ts @@ -1,4 +1,5 @@ import { assertEquals, fail } from "../../testing/asserts.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { rename, renameSync } from "./_fs_rename.ts"; import { existsSync } from "../../fs/mod.ts"; import { join, parse } from "../../path/mod.ts"; @@ -17,8 +18,7 @@ Deno.test({ .then(() => { assertEquals(existsSync(newPath), true); assertEquals(existsSync(file), false); - }) - .catch(() => fail()) + }, () => fail()) .finally(() => { if (existsSync(file)) Deno.removeSync(file); if (existsSync(newPath)) Deno.removeSync(newPath); @@ -36,3 +36,16 @@ Deno.test({ assertEquals(existsSync(file), false); }, }); + +Deno.test("[std/node/fs] rename callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_rename.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { rename } from ${JSON.stringify(importUrl)}`, + invocation: `rename(${JSON.stringify(tempFile)}, + ${JSON.stringify(`${tempFile}.newname`)}, `, + async cleanup() { + await Deno.remove(`${tempFile}.newname`); + }, + }); +}); diff --git a/node/_fs/_fs_rmdir.ts b/node/_fs/_fs_rmdir.ts index 2035a1e71788..e82e696e3d60 100644 --- a/node/_fs/_fs_rmdir.ts +++ b/node/_fs/_fs_rmdir.ts @@ -27,8 +27,7 @@ export function rmdir( if (!callback) throw new Error("No callback function supplied"); Deno.remove(path, { recursive: options?.recursive }) - .then((_) => callback()) - .catch(callback); + .then((_) => callback(), callback); } export function rmdirSync(path: string | URL, options?: rmdirOptions) { diff --git a/node/_fs/_fs_rmdir_test.ts b/node/_fs/_fs_rmdir_test.ts index 006bd7b9dac8..6f9c33274754 100644 --- a/node/_fs/_fs_rmdir_test.ts +++ b/node/_fs/_fs_rmdir_test.ts @@ -3,6 +3,7 @@ import { rmdir, rmdirSync } from "./_fs_rmdir.ts"; import { closeSync } from "./_fs_close.ts"; import { existsSync } from "../../fs/mod.ts"; import { join } from "../../path/mod.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; Deno.test({ name: "ASYNC: removing empty folder", @@ -14,8 +15,7 @@ Deno.test({ resolve(); }); }) - .then(() => assertEquals(existsSync(dir), false)) - .catch(() => fail()) + .then(() => assertEquals(existsSync(dir), false), () => fail()) .finally(() => { if (existsSync(dir)) Deno.removeSync(dir); }); @@ -58,8 +58,7 @@ Deno.test({ resolve(); }); }) - .then(() => assertEquals(existsSync(dir), false)) - .catch(() => fail()) + .then(() => assertEquals(existsSync(dir), false), () => fail()) .finally(() => { if (existsSync(dir)) Deno.removeSync(dir, { recursive: true }); const rAfter = Deno.resources(); @@ -86,3 +85,16 @@ Deno.test({ }, ignore: Deno.build.os === "windows", }); + +Deno.test("[std/node/fs] rmdir callback isn't called twice if error is thrown", async () => { + // The correct behaviour is not to catch any errors thrown, + // but that means there'll be an uncaught error and the test will fail. + // So the only way to test this is to spawn a subprocess, and succeed if it has a non-zero exit code. + // (assertThrowsAsync won't work because there's no way to catch the error.) + const tempDir = await Deno.makeTempDir(); + const importUrl = new URL("./_fs_rmdir.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { rmdir } from ${JSON.stringify(importUrl)}`, + invocation: `rmdir(${JSON.stringify(tempDir)}, `, + }); +}); diff --git a/node/_fs/_fs_stat.ts b/node/_fs/_fs_stat.ts index 3ca2a9918317..4f7903d16e24 100644 --- a/node/_fs/_fs_stat.ts +++ b/node/_fs/_fs_stat.ts @@ -234,11 +234,11 @@ export function CFISBIS(fileInfo: Deno.FileInfo, bigInt: boolean) { } export type statCallbackBigInt = ( - err: Error | undefined, + err: Error | null, stat: BigIntStats, ) => void; -export type statCallback = (err: Error | undefined, stat: Stats) => void; +export type statCallback = (err: Error | null, stat: Stats) => void; export function stat(path: string | URL, callback: statCallback): void; export function stat( @@ -260,8 +260,7 @@ export function stat( (typeof optionsOrCallback === "function" ? optionsOrCallback : maybeCallback) as ( - err: Error | undefined, - stat: BigIntStats | Stats, + ...args: [Error] | [null, BigIntStats | Stats] ) => void; const options = typeof optionsOrCallback === "object" ? optionsOrCallback @@ -269,9 +268,10 @@ export function stat( if (!callback) throw new Error("No callback function supplied"); - Deno.stat(path) - .then((stat) => callback(undefined, CFISBIS(stat, options.bigint))) - .catch((err) => callback(err, err)); + Deno.stat(path).then( + (stat) => callback(null, CFISBIS(stat, options.bigint)), + (err) => callback(err), + ); } export function statSync(path: string | URL): Stats; diff --git a/node/_fs/_fs_stat_test.ts b/node/_fs/_fs_stat_test.ts index 3ba8d1cb210f..e991985ea8d9 100644 --- a/node/_fs/_fs_stat_test.ts +++ b/node/_fs/_fs_stat_test.ts @@ -1,3 +1,4 @@ +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { BigIntStats, stat, Stats, statSync } from "./_fs_stat.ts"; import { assertEquals, fail } from "../../testing/asserts.ts"; @@ -68,8 +69,7 @@ Deno.test({ resolve(stat); }); }) - .then((stat) => assertStats(stat, Deno.statSync(file))) - .catch(() => fail()) + .then((stat) => assertStats(stat, Deno.statSync(file)), () => fail()) .finally(() => Deno.removeSync(file)); }, }); @@ -92,8 +92,10 @@ Deno.test({ resolve(stat); }); }) - .then((stat) => assertStatsBigInt(stat, Deno.statSync(file))) - .catch(() => fail()) + .then( + (stat) => assertStatsBigInt(stat, Deno.statSync(file)), + () => fail(), + ) .finally(() => Deno.removeSync(file)); }, }); @@ -105,3 +107,15 @@ Deno.test({ assertStatsBigInt(statSync(file, { bigint: true }), Deno.statSync(file)); }, }); + +Deno.test("[std/node/fs] stat callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_stat.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { stat } from ${JSON.stringify(importUrl)}`, + invocation: `stat(${JSON.stringify(tempFile)}, `, + async cleanup() { + await Deno.remove(tempFile); + }, + }); +}); diff --git a/node/_fs/_fs_unlink.ts b/node/_fs/_fs_unlink.ts index aba734fe13d1..7349bee46e3c 100644 --- a/node/_fs/_fs_unlink.ts +++ b/node/_fs/_fs_unlink.ts @@ -1,8 +1,6 @@ export function unlink(path: string | URL, callback: (err?: Error) => void) { if (!callback) throw new Error("No callback function supplied"); - Deno.remove(path) - .then((_) => callback()) - .catch(callback); + Deno.remove(path).then((_) => callback(), callback); } export function unlinkSync(path: string | URL) { diff --git a/node/_fs/_fs_unlink_test.ts b/node/_fs/_fs_unlink_test.ts index 46cde60b1e06..5021b1c380b7 100644 --- a/node/_fs/_fs_unlink_test.ts +++ b/node/_fs/_fs_unlink_test.ts @@ -1,5 +1,6 @@ import { assertEquals, fail } from "../../testing/asserts.ts"; import { existsSync } from "../../fs/mod.ts"; +import { assertCallbackErrorUncaught } from "../_utils.ts"; import { unlink, unlinkSync } from "./_fs_unlink.ts"; Deno.test({ @@ -12,8 +13,7 @@ Deno.test({ resolve(); }); }) - .then(() => assertEquals(existsSync(file), false)) - .catch(() => fail()) + .then(() => assertEquals(existsSync(file), false), () => fail()) .finally(() => { if (existsSync(file)) Deno.removeSync(file); }); @@ -28,3 +28,12 @@ Deno.test({ assertEquals(existsSync(file), false); }, }); + +Deno.test("[std/node/fs] unlink callback isn't called twice if error is thrown", async () => { + const tempFile = await Deno.makeTempFile(); + const importUrl = new URL("./_fs_unlink.ts", import.meta.url); + await assertCallbackErrorUncaught({ + prelude: `import { unlink } from ${JSON.stringify(importUrl)}`, + invocation: `unlink(${JSON.stringify(tempFile)}, `, + }); +}); diff --git a/node/_fs/_fs_watch_test.ts b/node/_fs/_fs_watch_test.ts index e85b4c9bc172..00fce4ffdce2 100644 --- a/node/_fs/_fs_watch_test.ts +++ b/node/_fs/_fs_watch_test.ts @@ -12,21 +12,15 @@ Deno.test({ async fn() { const file = Deno.makeTempFileSync(); const result: Array<[string, string]> = []; - await new Promise((resolve) => { - const watcher = watch( - file, - (eventType, filename) => result.push([eventType, filename]), - ); - wait(100) - .then(() => Deno.writeTextFileSync(file, "something")) - .then(() => wait(100)) - .then(() => watcher.close()) - .then(() => wait(100)) - .then(resolve); - }) - .then(() => { - assertEquals(result.length >= 1, true); - }) - .catch(() => fail()); + const watcher = watch( + file, + (eventType, filename) => result.push([eventType, filename]), + ); + await wait(100); + Deno.writeTextFileSync(file, "something"); + await wait(100); + watcher.close(); + await wait(100); + assertEquals(result.length >= 1, true); }, }); diff --git a/node/_utils.ts b/node/_utils.ts index 71ee0c7aa069..62a911843f94 100644 --- a/node/_utils.ts +++ b/node/_utils.ts @@ -1,6 +1,6 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. import { deferred } from "../async/mod.ts"; -import { fail } from "../testing/asserts.ts"; +import { assert, assertStringIncludes, fail } from "../testing/asserts.ts"; export type BinaryEncodings = "binary"; @@ -37,26 +37,28 @@ export type MaybeEmpty = T | null | undefined; export function intoCallbackAPI( // deno-lint-ignore no-explicit-any func: (...args: any[]) => Promise, - cb: MaybeEmpty<(err: MaybeNull, value: MaybeEmpty) => void>, + cb: MaybeEmpty<(err: MaybeNull, value?: MaybeEmpty) => void>, // deno-lint-ignore no-explicit-any ...args: any[] ): void { - func(...args) - .then((value) => cb && cb(null, value)) - .catch((err) => cb && cb(err, null)); + func(...args).then( + (value) => cb && cb(null, value), + (err) => cb && cb(err), + ); } export function intoCallbackAPIWithIntercept( // deno-lint-ignore no-explicit-any func: (...args: any[]) => Promise, interceptor: (v: T1) => T2, - cb: MaybeEmpty<(err: MaybeNull, value: MaybeEmpty) => void>, + cb: MaybeEmpty<(err: MaybeNull, value?: MaybeEmpty) => void>, // deno-lint-ignore no-explicit-any ...args: any[] ): void { - func(...args) - .then((value) => cb && cb(null, interceptor(value))) - .catch((err) => cb && cb(err, null)); + func(...args).then( + (value) => cb && cb(null, interceptor(value)), + (err) => cb && cb(err), + ); } export function spliceOne(list: string[], index: number): void { @@ -203,3 +205,43 @@ export function mustCall( callback, ]; } +/** Asserts that an error thrown in a callback will not be wrongly caught. */ +export async function assertCallbackErrorUncaught( + { prelude, invocation, cleanup }: { + /** Any code which needs to run before the actual invocation (notably, any import statements). */ + prelude?: string; + /** + * The start of the invocation of the function, e.g. `open("foo.txt", `. + * The callback will be added after it. + */ + invocation: string; + /** Called after the subprocess is finished but before running the assertions, e.g. to clean up created files. */ + cleanup?: () => Promise | void; + }, +) { + // Since the error has to be uncaught, and that will kill the Deno process, + // the only way to test this is to spawn a subprocess. + const p = Deno.run({ + cmd: [ + Deno.execPath(), + "eval", + "--no-check", // Running TSC for every one of these tests would take way too long + "--unstable", + `${prelude ?? ""} + + ${invocation}(err) => { + // If the bug is present and the callback is called again with an error, + // don't throw another error, so if the subprocess fails we know it had the correct behaviour. + if (!err) throw new Error("success"); + });`, + ], + stderr: "piped", + }); + const status = await p.status(); + const stderr = new TextDecoder().decode(await Deno.readAll(p.stderr)); + p.close(); + p.stderr.close(); + await cleanup?.(); + assert(!status.success); + assertStringIncludes(stderr, "Error: success"); +}