diagnostics_channel: ensure tracePromise consistency with non-Promises by Renegade334 · Pull Request #61766 · nodejs/node · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions doc/api/diagnostics_channel.md
30 changes: 19 additions & 11 deletions lib/diagnostics_channel.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ const {
ObjectDefineProperty,
ObjectGetPrototypeOf,
ObjectSetPrototypeOf,
Promise,
PromisePrototypeThen,
PromiseReject,
PromiseResolve,
ReflectApply,
SafeFinalizationRegistry,
SafeMap,
Expand Down Expand Up @@ -280,6 +276,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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing here avoids Promisifying thenables on rejection. The Promises/A+ standard dictates that throwing within the promise handlers results in the returned thenable rejecting. (https://promisesaplus.com/#point-42)

For native Promises, there should be no observable change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there already an existing testcase for this?
if not maybe a good idea to add one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's covered by

assert.rejects(
channel.tracePromise(common.mustCall(function(value) {
assert.deepStrictEqual(this, thisArg);
return Promise.reject(value);
}), input, thisArg, expectedError),
expectedError,
).then(common.mustCall());

}

function resolve(result) {
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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,
);
Loading