fix(node): Stop callbacks being called twice when callback throws err…

…or (denoland/deno#8867)

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.

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);
 }

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", ',
+  });
+});

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);
   }

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, ",
+  });
+});

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);
     });

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,12 +232,23 @@ 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 () => {
         await Deno.remove("_fs_appendFile_test_file.txt");
       });
   },
 });
+
+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);
+    },
+  });
+});

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);
 }
 
 /**

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);
+      },
+    });
+  },
+});

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);
 }
 
 /**

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);
+      },
+    });
+  },
+});