Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b4ea323

Browse filesBrowse files
Renegade334Renegade334
authored
diagnostics_channel: ensure tracePromise consistency with non-Promises
PR-URL: #61766 Fixes: #59936 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
1 parent a62f641 commit b4ea323
Copy full SHA for b4ea323

4 files changed

+143-20Lines changed: 143 additions & 20 deletions
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎doc/api/diagnostics_channel.md‎

Copy file name to clipboardExpand all lines: doc/api/diagnostics_channel.md
+23-9Lines changed: 23 additions & 9 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -829,23 +829,36 @@ channels.traceSync(() => {
829829
added:
830830
- v19.9.0
831831
- v18.19.0
832+
changes:
833+
- version: REPLACEME
834+
pr-url: https://github.com/nodejs/node/pull/61766
835+
description: Custom thenables will no longer be wrapped in native Promises.
836+
Non-thenables will be returned with a warning.
832837
-->
833838

834-
* `fn` {Function} Promise-returning function to wrap a trace around
839+
* `fn` {Function} Function to wrap a trace around
835840
* `context` {Object} Shared object to correlate trace events through
836841
* `thisArg` {any} The receiver to be used for the function call
837842
* `...args` {any} Optional arguments to pass to the function
838-
* Returns: {Promise} Chained from promise returned by the given function
839-
840-
Trace a promise-returning function call. This will always produce a
841-
[`start` event][] and [`end` event][] around the synchronous portion of the
842-
function execution, and will produce an [`asyncStart` event][] and
843-
[`asyncEnd` event][] when a promise continuation is reached. It may also
844-
produce an [`error` event][] if the given function throws an error or the
845-
returned promise rejects. This will run the given function using
843+
* Returns: {any} The return value of the given function, or the result of
844+
calling `.then(...)` on the return value if the tracing channel has active
845+
subscribers. If the return value is not a Promise or thenable, then
846+
it is returned as-is and a warning is emitted.
847+
848+
Trace an asynchronous function call which returns a {Promise} or
849+
[thenable object][]. This will always produce a [`start` event][] and
850+
[`end` event][] around the synchronous portion of the function execution, and
851+
will produce an [`asyncStart` event][] and [`asyncEnd` event][] when the
852+
returned promise is resolved or rejected. It may also produce an
853+
[`error` event][] if the given function throws an error or the returned promise
854+
is rejected. This will run the given function using
846855
[`channel.runStores(context, ...)`][] on the `start` channel which ensures all
847856
events should have any bound stores set to match this trace context.
848857

858+
If the value returned by `fn` is not a Promise or thenable, then it will be
859+
returned with a warning, and no `asyncStart` or `asyncEnd` events will be
860+
produced.
861+
849862
To ensure only correct trace graphs are formed, events will only be published
850863
if subscribers are present prior to starting the trace. Subscriptions which are
851864
added after the trace begins will not receive future events from that trace,
@@ -1526,3 +1539,4 @@ Emitted when a new thread is created.
15261539
[`start` event]: #startevent
15271540
[`worker_threads.locks`]: worker_threads.md#worker_threadslocks
15281541
[context loss]: async_context.md#troubleshooting-context-loss
1542+
[thenable object]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#thenables
Collapse file

‎lib/diagnostics_channel.js‎

Copy file name to clipboardExpand all lines: lib/diagnostics_channel.js
+19-11Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,6 @@ const {
1010
ObjectDefineProperty,
1111
ObjectGetPrototypeOf,
1212
ObjectSetPrototypeOf,
13-
Promise,
14-
PromisePrototypeThen,
15-
PromiseReject,
16-
PromiseResolve,
1713
ReflectApply,
1814
SafeFinalizationRegistry,
1915
SafeMap,
@@ -292,6 +288,11 @@ function tracingChannelFrom(nameOrChannels, name) {
292288
nameOrChannels);
293289
}
294290

291+
function emitNonThenableWarning(fn) {
292+
process.emitWarning(`tracePromise was called with the function '${fn.name || '<anonymous>'}', ` +
293+
'which returned a non-thenable.');
294+
}
295+
295296
class TracingChannel {
296297
constructor(nameOrChannels) {
297298
for (let i = 0; i < traceEvents.length; ++i) {
@@ -359,7 +360,11 @@ class TracingChannel {
359360

360361
tracePromise(fn, context = {}, thisArg, ...args) {
361362
if (!this.hasSubscribers) {
362-
return ReflectApply(fn, thisArg, args);
363+
const result = ReflectApply(fn, thisArg, args);
364+
if (typeof result?.then !== 'function') {
365+
emitNonThenableWarning(fn);
366+
}
367+
return result;
363368
}
364369

365370
const { start, end, asyncStart, asyncEnd, error } = this;
@@ -370,7 +375,7 @@ class TracingChannel {
370375
asyncStart.publish(context);
371376
// TODO: Is there a way to have asyncEnd _after_ the continuation?
372377
asyncEnd.publish(context);
373-
return PromiseReject(err);
378+
throw err;
374379
}
375380

376381
function resolve(result) {
@@ -383,12 +388,15 @@ class TracingChannel {
383388

384389
return start.runStores(context, () => {
385390
try {
386-
let promise = ReflectApply(fn, thisArg, args);
387-
// Convert thenables to native promises
388-
if (!(promise instanceof Promise)) {
389-
promise = PromiseResolve(promise);
391+
const result = ReflectApply(fn, thisArg, args);
392+
// If the return value is not a thenable, then return it with a warning.
393+
// Do not publish to asyncStart/asyncEnd.
394+
if (typeof result?.then !== 'function') {
395+
emitNonThenableWarning(fn);
396+
context.result = result;
397+
return result;
390398
}
391-
return PromisePrototypeThen(promise, resolve, reject);
399+
return result.then(resolve, reject);
392400
} catch (err) {
393401
context.error = err;
394402
error.publish(context);
Collapse file

‎test/parallel/test-diagnostics-channel-tracing-channel-promise-non-thenable.js‎

Copy file name to clipboard
+46Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const dc = require('diagnostics_channel');
5+
const assert = require('assert');
6+
7+
const channel = dc.tracingChannel('test');
8+
9+
const expectedResult = { foo: 'bar' };
10+
const input = { foo: 'bar' };
11+
const thisArg = { baz: 'buz' };
12+
13+
function checkStart(found) {
14+
assert.strictEqual(found, input);
15+
}
16+
17+
function checkEnd(found) {
18+
checkStart(found);
19+
assert.strictEqual(found.error, undefined);
20+
assert.deepStrictEqual(found.result, expectedResult);
21+
}
22+
23+
const handlers = {
24+
start: common.mustCall(checkStart),
25+
end: common.mustCall(checkEnd),
26+
asyncStart: common.mustNotCall(),
27+
asyncEnd: common.mustNotCall(),
28+
error: common.mustNotCall()
29+
};
30+
31+
channel.subscribe(handlers);
32+
33+
process.on('warning', common.mustCall((warning) => {
34+
assert.strictEqual(
35+
warning.message,
36+
"tracePromise was called with the function '<anonymous>', which returned a non-thenable."
37+
);
38+
}));
39+
40+
assert.strictEqual(
41+
channel.tracePromise(common.mustCall(function(value) {
42+
assert.deepStrictEqual(this, thisArg);
43+
return value;
44+
}), input, thisArg, expectedResult),
45+
expectedResult,
46+
);
Collapse file

‎test/parallel/test-diagnostics-channel-tracing-channel-promise-thenable.js‎

Copy file name to clipboard
+55Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const dc = require('diagnostics_channel');
5+
const assert = require('assert');
6+
7+
class ResolvedThenable {
8+
#result;
9+
constructor(value) {
10+
this.#result = value;
11+
}
12+
then(resolve) {
13+
return new ResolvedThenable(resolve(this.#result));
14+
}
15+
}
16+
17+
const channel = dc.tracingChannel('test');
18+
19+
const expectedResult = { foo: 'bar' };
20+
const input = { foo: 'bar' };
21+
const thisArg = { baz: 'buz' };
22+
23+
function check(found) {
24+
assert.strictEqual(found, input);
25+
}
26+
27+
function checkAsync(found) {
28+
check(found);
29+
assert.strictEqual(found.error, undefined);
30+
assert.deepStrictEqual(found.result, expectedResult);
31+
}
32+
33+
const handlers = {
34+
start: common.mustCall(check),
35+
end: common.mustCall(check),
36+
asyncStart: common.mustCall(checkAsync),
37+
asyncEnd: common.mustCall(checkAsync),
38+
error: common.mustNotCall()
39+
};
40+
41+
channel.subscribe(handlers);
42+
43+
let innerThenable;
44+
45+
const result = channel.tracePromise(common.mustCall(function(value) {
46+
assert.deepStrictEqual(this, thisArg);
47+
innerThenable = new ResolvedThenable(value);
48+
return innerThenable;
49+
}), input, thisArg, expectedResult);
50+
51+
assert(result instanceof ResolvedThenable);
52+
assert.notStrictEqual(result, innerThenable);
53+
result.then(common.mustCall((value) => {
54+
assert.deepStrictEqual(value, expectedResult);
55+
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.