diagnostics_channel: ensure tracePromise consistency with non-Promises

PR-URL: #61766
Fixes: #59936
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>

Author: Renegade334

doc/api/diagnostics_channel.md

@@ -829,23 +829,36 @@ channels.traceSync(() => {
 added:
  - v19.9.0
  - v18.19.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/61766
+    description: Custom thenables will no longer be wrapped in native Promises.
+                 Non-thenables will be returned with a warning.
 -->
 
-* `fn` {Function} Promise-returning function to wrap a trace around
+* `fn` {Function} Function to wrap a trace around
 * `context` {Object} Shared object to correlate trace events through
 * `thisArg` {any} The receiver to be used for the function call
 * `...args` {any} Optional arguments to pass to the function
-* Returns: {Promise} Chained from promise returned by the given function
-
-Trace a promise-returning function call. This will always produce a
-[`start` event][] and [`end` event][] around the synchronous portion of the
-function execution, and will produce an [`asyncStart` event][] and
-[`asyncEnd` event][] when a promise continuation is reached. It may also
-produce an [`error` event][] if the given function throws an error or the
-returned promise rejects. This will run the given function using
+* Returns: {any} The return value of the given function, or the result of
+  calling `.then(...)` on the return value if the tracing channel has active
+  subscribers. If the return value is not a Promise or thenable, then
+  it is returned as-is and a warning is emitted.
+
+Trace an asynchronous function call which returns a {Promise} or
+[thenable object][]. This will always produce a [`start` event][] and
+[`end` event][] around the synchronous portion of the function execution, and
+will produce an [`asyncStart` event][] and [`asyncEnd` event][] when the
+returned promise is resolved or rejected. It may also produce an
+[`error` event][] if the given function throws an error or the returned promise
+is rejected. This will run the given function using
 [`channel.runStores(context, ...)`][] on the `start` channel which ensures all
 events should have any bound stores set to match this trace context.
 
+If the value returned by `fn` is not a Promise or thenable, then it will be
+returned with a warning, and no `asyncStart` or `asyncEnd` events will be
+produced.
+
 To ensure only correct trace graphs are formed, events will only be published
 if subscribers are present prior to starting the trace. Subscriptions which are
 added after the trace begins will not receive future events from that trace,
@@ -1526,3 +1539,4 @@ Emitted when a new thread is created.
 [`start` event]: #startevent
 [`worker_threads.locks`]: worker_threads.md#worker_threadslocks
 [context loss]: async_context.md#troubleshooting-context-loss
+[thenable object]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#thenables

lib/diagnostics_channel.js

@@ -10,10 +10,6 @@ const {
   ObjectDefineProperty,
   ObjectGetPrototypeOf,
   ObjectSetPrototypeOf,
-  Promise,
-  PromisePrototypeThen,
-  PromiseReject,
-  PromiseResolve,
   ReflectApply,
   SafeFinalizationRegistry,
   SafeMap,
@@ -292,6 +288,11 @@ function tracingChannelFrom(nameOrChannels, name) {
                                  nameOrChannels);
 }
 
+function emitNonThenableWarning(fn) {
+  process.emitWarning(`tracePromise was called with the function '${fn.name || '<anonymous>'}', ` +
+                      'which returned a non-thenable.');
+}
+
 class TracingChannel {
   constructor(nameOrChannels) {
     for (let i = 0; i < traceEvents.length; ++i) {
@@ -359,7 +360,11 @@ class TracingChannel {
 
   tracePromise(fn, context = {}, thisArg, ...args) {
     if (!this.hasSubscribers) {
-      return ReflectApply(fn, thisArg, args);
+      const result = ReflectApply(fn, thisArg, args);
+      if (typeof result?.then !== 'function') {
+        emitNonThenableWarning(fn);
+      }
+      return result;
     }
 
     const { start, end, asyncStart, asyncEnd, error } = this;
@@ -370,7 +375,7 @@ class TracingChannel {
       asyncStart.publish(context);
       // TODO: Is there a way to have asyncEnd _after_ the continuation?
       asyncEnd.publish(context);
-      return PromiseReject(err);
+      throw err;
     }
 
     function resolve(result) {
@@ -383,12 +388,15 @@ class TracingChannel {
 
     return start.runStores(context, () => {
       try {
-        let promise = ReflectApply(fn, thisArg, args);
-        // Convert thenables to native promises
-        if (!(promise instanceof Promise)) {
-          promise = PromiseResolve(promise);
+        const result = ReflectApply(fn, thisArg, args);
+        // If the return value is not a thenable, then return it with a warning.
+        // Do not publish to asyncStart/asyncEnd.
+        if (typeof result?.then !== 'function') {
+          emitNonThenableWarning(fn);
+          context.result = result;
+          return result;
         }
-        return PromisePrototypeThen(promise, resolve, reject);
+        return result.then(resolve, reject);
       } catch (err) {
         context.error = err;
         error.publish(context);

test/parallel/test-diagnostics-channel-tracing-channel-promise-non-thenable.js

@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common');
+const dc = require('diagnostics_channel');
+const assert = require('assert');
+
+const channel = dc.tracingChannel('test');
+
+const expectedResult = { foo: 'bar' };
+const input = { foo: 'bar' };
+const thisArg = { baz: 'buz' };
+
+function checkStart(found) {
+  assert.strictEqual(found, input);
+}
+
+function checkEnd(found) {
+  checkStart(found);
+  assert.strictEqual(found.error, undefined);
+  assert.deepStrictEqual(found.result, expectedResult);
+}
+
+const handlers = {
+  start: common.mustCall(checkStart),
+  end: common.mustCall(checkEnd),
+  asyncStart: common.mustNotCall(),
+  asyncEnd: common.mustNotCall(),
+  error: common.mustNotCall()
+};
+
+channel.subscribe(handlers);
+
+process.on('warning', common.mustCall((warning) => {
+  assert.strictEqual(
+    warning.message,
+    "tracePromise was called with the function '<anonymous>', which returned a non-thenable."
+  );
+}));
+
+assert.strictEqual(
+  channel.tracePromise(common.mustCall(function(value) {
+    assert.deepStrictEqual(this, thisArg);
+    return value;
+  }), input, thisArg, expectedResult),
+  expectedResult,
+);

test/parallel/test-diagnostics-channel-tracing-channel-promise-thenable.js

@@ -0,0 +1,55 @@
+'use strict';
+
+const common = require('../common');
+const dc = require('diagnostics_channel');
+const assert = require('assert');
+
+class ResolvedThenable {
+  #result;
+  constructor(value) {
+    this.#result = value;
+  }
+  then(resolve) {
+    return new ResolvedThenable(resolve(this.#result));
+  }
+}
+
+const channel = dc.tracingChannel('test');
+
+const expectedResult = { foo: 'bar' };
+const input = { foo: 'bar' };
+const thisArg = { baz: 'buz' };
+
+function check(found) {
+  assert.strictEqual(found, input);
+}
+
+function checkAsync(found) {
+  check(found);
+  assert.strictEqual(found.error, undefined);
+  assert.deepStrictEqual(found.result, expectedResult);
+}
+
+const handlers = {
+  start: common.mustCall(check),
+  end: common.mustCall(check),
+  asyncStart: common.mustCall(checkAsync),
+  asyncEnd: common.mustCall(checkAsync),
+  error: common.mustNotCall()
+};
+
+channel.subscribe(handlers);
+
+let innerThenable;
+
+const result = channel.tracePromise(common.mustCall(function(value) {
+  assert.deepStrictEqual(this, thisArg);
+  innerThenable = new ResolvedThenable(value);
+  return innerThenable;
+}), input, thisArg, expectedResult);
+
+assert(result instanceof ResolvedThenable);
+assert.notStrictEqual(result, innerThenable);
+result.then(common.mustCall((value) => {
+  assert.deepStrictEqual(value, expectedResult);
+}));