Repository: nodejs/node
Pull request: diagnostics_channel: ensure tracePromise consistency with non-Promises

`tracingChannel.tracePromise()` is documented in the source code (albeit not in the docs) to accept functions that return non-Promise thenables. However, it currently has some wonky behaviour when the return value isn't a native Promise, and also in the undocumented case where the return value isn't promise-like at all.

What gets returned by `tracePromise()` is highly dependent on whether the tracing channel has active subscribers or not:

<table>
  <tr>
    <th rowspan="2"><code>result</code> type</th>
    <th colspan="2">Without subscribers</th>
    <th colspan="2">With subscribers</th>
  </tr>
  <tr>
    <th>Return value</th>
    <th>Return type</th>
    <th>Return value</th>
    <th>Return type</th>
  </tr>
  <tr>
    <td>Native Promise</td>
    <td><code>result</code></td>
    <td>Native Promise</td>
    <td><code>result.then(...)</code></td>
    <td>Native Promise</td>
  </tr>
  <tr>
    <td>Custom thenable</td>
    <td><code>result</code></td>
    <td>Custom thenable</td>
    <td><code>Promise.resolve(result).then(...)</code></td>
    <td>Native Promise</td>
  </tr>
  <tr>
    <td>Non-thenable</td>
    <td><code>result</code></td>
    <td>Non-thenable</td>
    <td><code>Promise.resolve(result).then(...)</code></td>
    <td>Native Promise</td>
  </tr>
</table>

The major inconsistencies are:
- If the wrapped function returns a custom thenable, then it will either be returned as a custom thenable or wrapped in a native Promise, depending on whether there are active subscribers.
- If the wrapped function returns a non-thenable, then it will either be returned verbatim or wrapped in a resolved native Promise, depending on whether there are active subscribers.
- In the case where the wrapped function synchronously returns a non-thenable, then tracePromise produces `asyncStart`/`asyncEnd` events, despite no asynchronous execution. On the other hand, if the wrapped function _throws_ synchronously, these events are not produced.

This PR makes some changes to the non-Promise cases to make the behaviours consistent.
- If the wrapped function returns either a Promise or a custom thenable, then the value returned by tracePromise will have the same type: either the original value (if no subscribers) or the value of `result.then(...)` (if active subscribers).
- If the wrapped function returns a non-thenable result, then it is returned verbatim, and a warning is emitted. No `asyncStart` or `asyncEnd` events are produced.

<table>
  <tr>
    <th rowspan="2"><code>result</code> type</th>
    <th colspan="2">Without subscribers</th>
    <th colspan="2">With subscribers</th>
  </tr>
  <tr>
    <th>Return value</th>
    <th>Return type</th>
    <th>Return value</th>
    <th>Return type</th>
  </tr>
  <tr>
    <td>Native Promise</td>
    <td><code>result</code></td>
    <td>Native Promise</td>
    <td><code>result.then(...)</code></td>
    <td>Native Promise</td>
  </tr>
  <tr>
    <td>Custom thenable</td>
    <td><code>result</code></td>
    <td>Custom thenable</td>
    <td><code>result.then(...)</code></td>
    <td>Custom thenable</td>
  </tr>
  <tr>
    <td>Non-thenable</td>
    <td><code>result</code></td>
    <td>Non-thenable</td>
    <td><code>result</code></td>
    <td>Non-thenable</td>
  </tr>
</table>

Fixes: #59936

Changed files:
filename: doc/api/diagnostics_channel.md
patch: @@ -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

filename: lib/diagnostics_channel.js
patch: @@ -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);

filename: test/parallel/test-diagnostics-channel-tracing-channel-promise-non-thenable.js
patch: @@ -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,
+);

filename: test/parallel/test-diagnostics-channel-tracing-channel-promise-thenable.js
patch: @@ -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);
+}));