GeezerVison is showing github.com — diagnostics_channel: ensure tracePromise consistency with non-Promises · nodejs/node@b4ea323
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file 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 number | Diff line number | Diff line change |
|---|
@@ -829,23 +829,36 @@ channels.traceSync(() => { | 829 | 829 | added: | 830 | 830 | - v19.9.0 | 831 | 831 | - 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. | 832 | 837 | --> | 833 | 838 |
| 834 | | -* `fn` {Function} Promise-returning function to wrap a trace around | | 839 | +* `fn` {Function} Function to wrap a trace around | 835 | 840 | * `context` {Object} Shared object to correlate trace events through | 836 | 841 | * `thisArg` {any} The receiver to be used for the function call | 837 | 842 | * `...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 | 846 | 855 | [`channel.runStores(context, ...)`][] on the `start` channel which ensures all | 847 | 856 | events should have any bound stores set to match this trace context. | 848 | 857 |
| | 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 | + | 849 | 862 | To ensure only correct trace graphs are formed, events will only be published | 850 | 863 | if subscribers are present prior to starting the trace. Subscriptions which are | 851 | 864 | added after the trace begins will not receive future events from that trace, | @@ -1526,3 +1539,4 @@ Emitted when a new thread is created. | 1526 | 1539 | [`start` event]: #startevent | 1527 | 1540 | [`worker_threads.locks`]: worker_threads.md#worker_threadslocks | 1528 | 1541 | [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 Copy file name to clipboardExpand all lines: lib/diagnostics_channel.js +19-11Lines changed: 19 additions & 11 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -10,10 +10,6 @@ const { | 10 | 10 | ObjectDefineProperty, | 11 | 11 | ObjectGetPrototypeOf, | 12 | 12 | ObjectSetPrototypeOf, | 13 | | - Promise, | 14 | | - PromisePrototypeThen, | 15 | | - PromiseReject, | 16 | | - PromiseResolve, | 17 | 13 | ReflectApply, | 18 | 14 | SafeFinalizationRegistry, | 19 | 15 | SafeMap, | @@ -292,6 +288,11 @@ function tracingChannelFrom(nameOrChannels, name) { | 292 | 288 | nameOrChannels); | 293 | 289 | } | 294 | 290 |
| | 291 | +function emitNonThenableWarning(fn) { | | 292 | + process.emitWarning(`tracePromise was called with the function '${fn.name || '<anonymous>'}', ` + | | 293 | + 'which returned a non-thenable.'); | | 294 | +} | | 295 | + | 295 | 296 | class TracingChannel { | 296 | 297 | constructor(nameOrChannels) { | 297 | 298 | for (let i = 0; i < traceEvents.length; ++i) { | @@ -359,7 +360,11 @@ class TracingChannel { | 359 | 360 |
| 360 | 361 | tracePromise(fn, context = {}, thisArg, ...args) { | 361 | 362 | 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; | 363 | 368 | } | 364 | 369 |
| 365 | 370 | const { start, end, asyncStart, asyncEnd, error } = this; | @@ -370,7 +375,7 @@ class TracingChannel { | 370 | 375 | asyncStart.publish(context); | 371 | 376 | // TODO: Is there a way to have asyncEnd _after_ the continuation? | 372 | 377 | asyncEnd.publish(context); | 373 | | - return PromiseReject(err); | | 378 | + throw err; | 374 | 379 | } | 375 | 380 |
| 376 | 381 | function resolve(result) { | @@ -383,12 +388,15 @@ class TracingChannel { | 383 | 388 |
| 384 | 389 | return start.runStores(context, () => { | 385 | 390 | 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; | 390 | 398 | } | 391 | | - return PromisePrototypeThen(promise, resolve, reject); | | 399 | + return result.then(resolve, reject); | 392 | 400 | } catch (err) { | 393 | 401 | context.error = err; | 394 | 402 | error.publish(context); | |
Collapse file Copy file name to clipboard +46Lines changed: 46 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 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 Copy file name to clipboard +55Lines changed: 55 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 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 | +})); |
|