diagnostics_channel: ensure tracePromise return values are unmodified

Previously, the return value of tracePromise would depend on the
presence or absence of subscribers.

Author: Renegade334

doc/api/diagnostics_channel.md

@@ -831,14 +831,15 @@ added:
 * `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
+* Returns: {any} The return value of 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
+function execution. If the return value is a {Promise} or [thenable][], it will
+also produce an [`asyncStart` event][] and [`asyncEnd` event][] when the
+returned promise is fulfilled or rejected. 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
 [`channel.runStores(context, ...)`][] on the `start` channel which ensures all
 events should have any bound stores set to match this trace context.
 
@@ -1436,3 +1437,4 @@ Emitted when a new thread is created.
 [`process.execve()`]: process.md#processexecvefile-args-env
 [`start` event]: #startevent
 [context loss]: async_context.md#troubleshooting-context-loss
+[thenable]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#thenables

lib/diagnostics_channel.js

@@ -10,9 +10,7 @@ const {
   ObjectDefineProperty,
   ObjectGetPrototypeOf,
   ObjectSetPrototypeOf,
-  Promise,
   PromisePrototypeThen,
-  PromiseReject,
   PromiseResolve,
   ReflectApply,
   SafeFinalizationRegistry,
@@ -25,6 +23,9 @@ const {
     ERR_INVALID_ARG_TYPE,
   },
 } = require('internal/errors');
+const {
+  isPromise,
+} = require('internal/util/types');
 const {
   validateFunction,
 } = require('internal/validators');
@@ -358,25 +359,28 @@ class TracingChannel {
       asyncStart.publish(context);
       // TODO: Is there a way to have asyncEnd _after_ the continuation?
       asyncEnd.publish(context);
-      return PromiseReject(err);
     }
 
     function resolve(result) {
       context.result = result;
       asyncStart.publish(context);
       // TODO: Is there a way to have asyncEnd _after_ the continuation?
       asyncEnd.publish(context);
-      return result;
     }
 
     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 result isn't a Promise or thenable, then make this the
+        // trace result immediately.
+        // Otherwise, register the async tracing callbacks, then return the
+        // original promise.
+        if (!isPromise(result) && typeof result?.then !== 'function') {
+          context.result = result;
+        } else {
+          PromisePrototypeThen(PromiseResolve(result), resolve, reject);
         }
-        return PromisePrototypeThen(promise, resolve, reject);
+        return result;
       } catch (err) {
         context.error = err;
         error.publish(context);

test/fixtures/source-map/output/source_map_assert_source_line.snapshot

@@ -7,7 +7,7 @@ AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
     *
     *
     *
-    at TracingChannel.traceSync (node:diagnostics_channel:328:14)
+    at TracingChannel.traceSync (node:diagnostics_channel:329:14)
     *
     *
     *

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

@@ -0,0 +1,88 @@
+'use strict';
+
+const common = require('../common');
+const dc = require('diagnostics_channel');
+const assert = require('assert');
+
+const kSuccess = Symbol('kSuccess');
+const kFailure = Symbol('kFailure');
+
+class DerivedPromise extends Promise {}
+
+const fulfilledThenable = {
+  then(resolve) {
+    resolve(kSuccess);
+    return this;
+  },
+};
+const rejectedThenable = {
+  then(_, reject) {
+    reject(kFailure);
+    return this;
+  },
+};
+
+function check(event) {
+  if (event.expectError) {
+    assert.strictEqual(event.error, kFailure);
+    assert.strictEqual(event.result, undefined);
+  } else {
+    assert.strictEqual(event.error, undefined);
+    assert.strictEqual(event.result, kSuccess);
+  }
+}
+
+let i = 0;
+
+function test(expected, { withSubscribers, expectAsync, expectError }) {
+  const channel = dc.tracingChannel(`test${i++}`);
+
+  if (withSubscribers) {
+    channel.subscribe({
+      start: common.mustCall(),
+      end: common.mustCall(),
+      asyncStart: expectAsync ? common.mustCall(check) : common.mustNotCall(),
+      asyncEnd: expectAsync ? common.mustCall(check) : common.mustNotCall(),
+      error: expectError ? common.mustCall(check) : common.mustNotCall(),
+    });
+  }
+
+  const result = channel.tracePromise(() => expected, { expectError });
+  assert.strictEqual(result, expected);
+}
+
+// Should trace Promises and thenables, and return them unaltered
+const testValues = [
+  [DerivedPromise.resolve(kSuccess), false],
+  [DerivedPromise.reject(kFailure), true],
+  [fulfilledThenable, false],
+  [rejectedThenable, true],
+];
+for (const [value, expectError] of testValues) {
+  test(value, { withSubscribers: false });
+  test(value, { withSubscribers: true, expectAsync: true, expectError });
+}
+
+// Should return non-thenable values unaltered, and should not call async handlers
+test(kSuccess, { withSubscribers: false });
+test(kSuccess, { withSubscribers: true, expectAsync: false, expectError: false });
+
+// Should throw synchronous exceptions unaltered
+{
+  const channel = dc.tracingChannel(`test${i++}`);
+
+  const error = new Error('Synchronous exception');
+  const throwError = () => { throw error; };
+
+  assert.throws(() => channel.tracePromise(throwError), error);
+
+  channel.subscribe({
+    start: common.mustCall(),
+    end: common.mustCall(),
+    asyncStart: common.mustNotCall(),
+    asyncEnd: common.mustNotCall(),
+    error: common.mustCall(),
+  });
+
+  assert.throws(() => channel.tracePromise(throwError), error);
+}