From bf941af74acf81aa6700d0240a84d9fc127a928d Mon Sep 17 00:00:00 2001 From: Renegade334 Date: Sat, 20 Sep 2025 08:37:51 +0100 Subject: [PATCH] diagnostics_channel: ensure tracePromise return values are unmodified Previously, the return value of tracePromise would depend on the presence or absence of subscribers. --- doc/api/diagnostics_channel.md | 12 +-- lib/diagnostics_channel.js | 22 +++-- .../source_map_assert_source_line.snapshot | 2 +- ...channel-tracing-channel-promise-returns.js | 88 +++++++++++++++++++ 4 files changed, 109 insertions(+), 15 deletions(-) create mode 100644 test/parallel/test-diagnostics-channel-tracing-channel-promise-returns.js diff --git a/doc/api/diagnostics_channel.md b/doc/api/diagnostics_channel.md index 771fa4efe01429..e65d0cd6346174 100644 --- a/doc/api/diagnostics_channel.md +++ b/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 diff --git a/lib/diagnostics_channel.js b/lib/diagnostics_channel.js index 3deb301e7f3cd2..230ca0c90241a2 100644 --- a/lib/diagnostics_channel.js +++ b/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,7 +359,6 @@ 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) { @@ -366,17 +366,21 @@ class TracingChannel { 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); diff --git a/test/fixtures/source-map/output/source_map_assert_source_line.snapshot b/test/fixtures/source-map/output/source_map_assert_source_line.snapshot index 9def6eb4d7bedb..addc1bd23944f4 100644 --- a/test/fixtures/source-map/output/source_map_assert_source_line.snapshot +++ b/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) * * * diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-promise-returns.js b/test/parallel/test-diagnostics-channel-tracing-channel-promise-returns.js new file mode 100644 index 00000000000000..e484909283956e --- /dev/null +++ b/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); +}