fix(node): util.callbackify (#22200)

Fixes #22180 Matches the Node.js implementation more closely. Removed types, they do not help just make it harder to debug with stack traces.
denoland · Feb 1, 2024 · 4cab054 · 4cab054
1 parent e2f30ef
commit 4cab054
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 80 deletions.
diff --git a/cli/tests/unit_node/util_test.ts b/cli/tests/unit_node/util_test.ts
@@ -292,3 +292,26 @@ Deno.test({
     fn();
   },
 });
+
+Deno.test({
+  name: "[util] callbackify() works",
+  fn() {
+    const fn = util.callbackify(() => Promise.resolve("foo"));
+    fn((err, value) => {
+      assert(err === null);
+      assert(value === "foo");
+    });
+  },
+});
+
+Deno.test({
+  name: "[util] callbackify(undefined) throws",
+  fn() {
+    assertThrows(
+      // @ts-expect-error: testing runtime error
+      () => util.callbackify(undefined),
+      TypeError,
+      'The "original" argument must be of type function',
+    );
+  },
+});
diff --git a/ext/node/lib.rs b/ext/node/lib.rs
@@ -386,7 +386,7 @@ deno_core::extension!(deno_node,
     "_stream.mjs",
     "_tls_common.ts",
     "_tls_wrap.ts",
-    "_util/_util_callbackify.ts",
+    "_util/_util_callbackify.js",
     "_util/asserts.ts",
     "_util/async.ts",
     "_util/os.ts",

diff --git a/...node/polyfills/_util/_util_callbackify.ts → ...node/polyfills/_util/_util_callbackify.js b/...node/polyfills/_util/_util_callbackify.ts → ...node/polyfills/_util/_util_callbackify.js
@@ -26,105 +26,48 @@
 import { primordials } from "ext:core/mod.js";
 const {
   ArrayPrototypePop,
-  ReflectApply,
   Error,
   FunctionPrototypeBind,
+  ReflectApply,
   ObjectDefineProperties,
   ObjectGetOwnPropertyDescriptors,
+  ObjectSetPrototypeOf,
+  ObjectValues,
   PromisePrototypeThen,
-  TypeError,
 } = primordials;
 
 import { nextTick } from "ext:deno_node/_next_tick.ts";
+import { validateFunction } from "ext:deno_node/internal/validators.mjs";
 
 class NodeFalsyValueRejectionError extends Error {
-  public reason: unknown;
-  public code = "ERR_FALSY_VALUE_REJECTION";
-  constructor(reason: unknown) {
+  code = "ERR_FALSY_VALUE_REJECTION";
+  constructor(reason) {
     super("Promise was rejected with falsy value");
     this.reason = reason;
   }
 }
-class NodeInvalidArgTypeError extends TypeError {
-  public code = "ERR_INVALID_ARG_TYPE";
-  constructor(argumentName: string) {
-    super(`The ${argumentName} argument must be of type function.`);
-  }
-}
-
-type Callback<ResultT> =
-  | ((err: Error) => void)
-  | ((err: null, result: ResultT) => void);
 
-function callbackify<ResultT>(
-  fn: () => PromiseLike<ResultT>,
-): (callback: Callback<ResultT>) => void;
-function callbackify<ArgT, ResultT>(
-  fn: (arg: ArgT) => PromiseLike<ResultT>,
-): (arg: ArgT, callback: Callback<ResultT>) => void;
-function callbackify<Arg1T, Arg2T, ResultT>(
-  fn: (arg1: Arg1T, arg2: Arg2T) => PromiseLike<ResultT>,
-): (arg1: Arg1T, arg2: Arg2T, callback: Callback<ResultT>) => void;
-function callbackify<Arg1T, Arg2T, Arg3T, ResultT>(
-  fn: (arg1: Arg1T, arg2: Arg2T, arg3: Arg3T) => PromiseLike<ResultT>,
-): (arg1: Arg1T, arg2: Arg2T, arg3: Arg3T, callback: Callback<ResultT>) => void;
-function callbackify<Arg1T, Arg2T, Arg3T, Arg4T, ResultT>(
-  fn: (
-    arg1: Arg1T,
-    arg2: Arg2T,
-    arg3: Arg3T,
-    arg4: Arg4T,
-  ) => PromiseLike<ResultT>,
-): (
-  arg1: Arg1T,
-  arg2: Arg2T,
-  arg3: Arg3T,
-  arg4: Arg4T,
-  callback: Callback<ResultT>,
-) => void;
-function callbackify<Arg1T, Arg2T, Arg3T, Arg4T, Arg5T, ResultT>(
-  fn: (
-    arg1: Arg1T,
-    arg2: Arg2T,
-    arg3: Arg3T,
-    arg4: Arg4T,
-    arg5: Arg5T,
-  ) => PromiseLike<ResultT>,
-): (
-  arg1: Arg1T,
-  arg2: Arg2T,
-  arg3: Arg3T,
-  arg4: Arg4T,
-  arg5: Arg5T,
-  callback: Callback<ResultT>,
-) => void;
+function callbackify(original) {
+  validateFunction(original, "original");
 
-function callbackify<ResultT>(
-  original: (...args: unknown[]) => PromiseLike<ResultT>,
-): (...args: unknown[]) => void {
-  if (typeof original !== "function") {
-    throw new NodeInvalidArgTypeError('"original"');
-  }
-
-  const callbackified = function (this: unknown, ...args: unknown[]) {
+  // We DO NOT return the promise as it gives the user a false sense that
+  // the promise is actually somehow related to the callback's execution
+  // and that the callback throwing will reject the promise.
+  function callbackified(...args) {
     const maybeCb = ArrayPrototypePop(args);
-    if (typeof maybeCb !== "function") {
-      throw new NodeInvalidArgTypeError("last");
-    }
-    const cb = (...args: unknown[]) => {
-      ReflectApply(maybeCb, this, args);
-    };
+    validateFunction(maybeCb, "last argument");
+    const cb = FunctionPrototypeBind(maybeCb, this);
+    // In true node style we process the callback on `nextTick` with all the
+    // implications (stack, `uncaughtException`, `async_hooks`)
     PromisePrototypeThen(
-      ReflectApply(this, args),
-      (ret: unknown) => {
-        nextTick(FunctionPrototypeBind(cb, this, null, ret));
-      },
-      (rej: unknown) => {
+      ReflectApply(original, this, args),
+      (ret) => nextTick(cb, null, ret),
+      (rej) => {
         rej = rej || new NodeFalsyValueRejectionError(rej);
-        nextTick(FunctionPrototypeBind(cb, this, rej));
+        return nextTick(cb, rej);
       },
     );
-  };
+  }
 
   const descriptors = ObjectGetOwnPropertyDescriptors(original);
   // It is possible to manipulate a functions `length` or `name` property. This
@@ -135,6 +78,12 @@ function callbackify<ResultT>(
   if (typeof descriptors.name.value === "string") {
     descriptors.name.value += "Callbackified";
   }
+  const propertiesValues = ObjectValues(descriptors);
+  for (let i = 0; i < propertiesValues.length; i++) {
+    // We want to use null-prototype objects to not rely on globally mutable
+    // %Object.prototype%.
+    ObjectSetPrototypeOf(propertiesValues[i], null);
+  }
   ObjectDefineProperties(callbackified, descriptors);
   return callbackified;
 }

diff --git a/ext/node/polyfills/util.ts b/ext/node/polyfills/util.ts
@@ -28,7 +28,7 @@ const {
 } = primordials;
 
 import { promisify } from "ext:deno_node/internal/util.mjs";
-import { callbackify } from "ext:deno_node/_util/_util_callbackify.ts";
+import { callbackify } from "ext:deno_node/_util/_util_callbackify.js";
 import { debuglog } from "ext:deno_node/internal/util/debuglog.ts";
 import {
   format,