diff --git a/doc/api/diagnostics_channel.md b/doc/api/diagnostics_channel.md index e9ac279cc62917..653576f72bf79e 100644 --- a/doc/api/diagnostics_channel.md +++ b/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, @@ -1457,3 +1470,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 object]: 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..12be7209948b6d 100644 --- a/lib/diagnostics_channel.js +++ b/lib/diagnostics_channel.js @@ -10,10 +10,6 @@ const { ObjectDefineProperty, ObjectGetPrototypeOf, ObjectSetPrototypeOf, - Promise, - PromisePrototypeThen, - PromiseReject, - PromiseResolve, ReflectApply, SafeFinalizationRegistry, SafeMap, @@ -280,6 +276,11 @@ function tracingChannelFrom(nameOrChannels, name) { nameOrChannels); } +function emitNonThenableWarning(fn) { + process.emitWarning(`tracePromise was called with the function '${fn.name || ''}', ` + + 'which returned a non-thenable.'); +} + class TracingChannel { constructor(nameOrChannels) { for (let i = 0; i < traceEvents.length; ++i) { @@ -347,7 +348,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; @@ -358,7 +363,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) { @@ -371,12 +376,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); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-promise-non-thenable.js b/test/parallel/test-diagnostics-channel-tracing-channel-promise-non-thenable.js new file mode 100644 index 00000000000000..dc00e0e5a97c67 --- /dev/null +++ b/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 '', which returned a non-thenable." + ); +})); + +assert.strictEqual( + channel.tracePromise(common.mustCall(function(value) { + assert.deepStrictEqual(this, thisArg); + return value; + }), input, thisArg, expectedResult), + expectedResult, +); diff --git a/test/parallel/test-diagnostics-channel-tracing-channel-promise-thenable.js b/test/parallel/test-diagnostics-channel-tracing-channel-promise-thenable.js new file mode 100644 index 00000000000000..b93be1dd304c7d --- /dev/null +++ b/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); +}));