fix(std/node): Stop callbacks being called twice when callback throws error (#8867)

1 files changed, 51 insertions, 9 deletions

diff --git a/std/node/_utils.ts b/std/node/_utils.ts
index 71ee0c7aa..62a911843 100644
--- a/std/node/_utils.ts
+++ b/std/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> = T | null | undefined;
 export function intoCallbackAPI<T>(
   // deno-lint-ignore no-explicit-any
   func: (...args: any[]) => Promise<T>,
-  cb: MaybeEmpty<(err: MaybeNull<Error>, value: MaybeEmpty<T>) => void>,
+  cb: MaybeEmpty<(err: MaybeNull<Error>, value?: MaybeEmpty<T>) => 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<T1, T2>(
   // deno-lint-ignore no-explicit-any
   func: (...args: any[]) => Promise<T1>,
   interceptor: (v: T1) => T2,
-  cb: MaybeEmpty<(err: MaybeNull<Error>, value: MaybeEmpty<T2>) => void>,
+  cb: MaybeEmpty<(err: MaybeNull<Error>, value?: MaybeEmpty<T2>) => 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<T extends unknown[]>(
     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> | 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");
+}