GeezerVison is showing github.com — diagnostics_channel: return original thenable · nodejs/node@dc052c0
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 +9-6Lines changed: 9 additions & 6 deletions - Display the source diff
- Display the rich diff
| Original file line number | Diff line number | Diff line change |
|---|
@@ -948,20 +948,23 @@ added: | 948 | 948 | - v19.9.0 | 949 | 949 | - v18.19.0 | 950 | 950 | changes: | | 951 | + - version: REPLACEME | | 952 | + pr-url: https://github.com/nodejs/node/pull/62407 | | 953 | + description: Non-native-Promise thenables are now returned as-is, | | 954 | + preserving their original type and methods. | 951 | 955 | - version: v26.0.0 | 952 | 956 | pr-url: https://github.com/nodejs/node/pull/61766 | 953 | | - description: Custom thenables will no longer be wrapped in native Promises. | 954 | | - Non-thenables will be returned with a warning. | | 957 | + description: Non-thenables will be returned with a warning. | 955 | 958 | --> | 956 | 959 |
| 957 | 960 | * `fn` {Function} Function to wrap a trace around | 958 | 961 | * `context` {Object} Shared object to correlate trace events through | 959 | 962 | * `thisArg` {any} The receiver to be used for the function call | 960 | 963 | * `...args` {any} Optional arguments to pass to the function | 961 | | -* Returns: {any} The return value of the given function, or the result of | 962 | | - calling `.then(...)` on the return value if the tracing channel has active | 963 | | - subscribers. If the return value is not a Promise or thenable, then | 964 | | - it is returned as-is and a warning is emitted. | | 964 | +* Returns: {any} The return value of the given function. If the return value | | 965 | + is a Promise or thenable, tracing events will be published when it settles. | | 966 | + If the return value is not a Promise or thenable, it is returned as-is and | | 967 | + a warning is emitted. | 965 | 968 |
| 966 | 969 | Trace an asynchronous function call which returns a {Promise} or | 967 | 970 | [thenable object][]. This will always produce a [`start` event][] and | |
Collapse file Copy file name to clipboardExpand all lines: lib/diagnostics_channel.js +18-9Lines changed: 18 additions & 9 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -10,8 +10,8 @@ const { | 10 | 10 | ObjectDefineProperty, | 11 | 11 | ObjectGetPrototypeOf, | 12 | 12 | ObjectSetPrototypeOf, | | 13 | + PromisePrototype, | 13 | 14 | PromisePrototypeThen, | 14 | | - PromiseReject, | 15 | 15 | ReflectApply, | 16 | 16 | SafeFinalizationRegistry, | 17 | 17 | SafeMap, | @@ -546,17 +546,21 @@ class TracingChannel { | 546 | 546 | const { error } = this; | 547 | 547 | const continuationWindow = this.#continuationWindow; | 548 | 548 |
| 549 | | - function reject(err) { | | 549 | + function onReject(err) { | 550 | 550 | context.error = err; | 551 | 551 | error.publish(context); | 552 | 552 | // Use continuation window for asyncStart/asyncEnd | 553 | 553 | // eslint-disable-next-line no-unused-vars | 554 | 554 | using scope = continuationWindow.withScope(context); | 555 | 555 | // TODO: Is there a way to have asyncEnd _after_ the continuation? | 556 | | - return PromiseReject(err); | 557 | 556 | } | 558 | 557 |
| 559 | | - function resolve(result) { | | 558 | + function onRejectWithRethrow(err) { | | 559 | + onReject(err); | | 560 | + throw err; | | 561 | + } | | 562 | + | | 563 | + function onResolve(result) { | 560 | 564 | context.result = result; | 561 | 565 | // Use continuation window for asyncStart/asyncEnd | 562 | 566 | // eslint-disable-next-line no-unused-vars | @@ -576,12 +580,17 @@ class TracingChannel { | 576 | 580 | context.result = result; | 577 | 581 | return result; | 578 | 582 | } | 579 | | - // For native Promises use PromisePrototypeThen to avoid user overrides. | 580 | | - if (isPromise(result)) { | 581 | | - return PromisePrototypeThen(result, resolve, reject); | | 583 | + // isPromise() matches sub-classes, but we need to match only direct | | 584 | + // instances of the native Promise type to safely use PromisePrototypeThen. | | 585 | + if (isPromise(result) && ObjectGetPrototypeOf(result) === PromisePrototype) { | | 586 | + return PromisePrototypeThen(result, onResolve, onRejectWithRethrow); | 582 | 587 | } | 583 | | - // For custom thenables, call .then() directly to preserve the thenable type. | 584 | | - return result.then(resolve, reject); | | 588 | + // For non-native thenables, subscribe to the result but return the | | 589 | + // original thenable so the consumer can continue handling it directly. | | 590 | + // Non-native thenables don't have unhandledRejection tracking, so | | 591 | + // swallowing the rejection here doesn't change existing behaviour. | | 592 | + result.then(onResolve, onReject); | | 593 | + return result; | 585 | 594 | } catch (err) { | 586 | 595 | context.error = err; | 587 | 596 | error.publish(context); | |
Collapse file Copy file name to clipboard +52Lines changed: 52 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 SpoofedPromise extends Promise { | | 8 | + customMethod() { | | 9 | + return 'works'; | | 10 | + } | | 11 | +} | | 12 | + | | 13 | +const channel = dc.tracingChannel('test'); | | 14 | + | | 15 | +const expectedResult = { foo: 'bar' }; | | 16 | +const input = { foo: 'bar' }; | | 17 | +const thisArg = { baz: 'buz' }; | | 18 | + | | 19 | +function check(found) { | | 20 | + assert.strictEqual(found, input); | | 21 | +} | | 22 | + | | 23 | +function checkAsync(found) { | | 24 | + check(found); | | 25 | + assert.strictEqual(found.error, undefined); | | 26 | + assert.deepStrictEqual(found.result, expectedResult); | | 27 | +} | | 28 | + | | 29 | +const handlers = { | | 30 | + start: common.mustCall(check), | | 31 | + end: common.mustCall(check), | | 32 | + asyncStart: common.mustCall(checkAsync), | | 33 | + asyncEnd: common.mustCall(checkAsync), | | 34 | + error: common.mustNotCall() | | 35 | +}; | | 36 | + | | 37 | +channel.subscribe(handlers); | | 38 | + | | 39 | +let innerPromise; | | 40 | + | | 41 | +const result = channel.tracePromise(common.mustCall(function() { | | 42 | + innerPromise = SpoofedPromise.resolve(expectedResult); | | 43 | + // Spoof the constructor to try to trick the brand check | | 44 | + innerPromise.constructor = Promise; | | 45 | + return innerPromise; | | 46 | +}), input, thisArg); | | 47 | + | | 48 | +// Despite the spoofed constructor, the original subclass instance should be | | 49 | +// returned directly so that custom methods remain accessible. | | 50 | +assert(result instanceof SpoofedPromise); | | 51 | +assert.strictEqual(result, innerPromise); | | 52 | +assert.strictEqual(result.customMethod(), 'works'); |
Collapse file Copy file name to clipboardExpand all lines: test/parallel/test-diagnostics-channel-tracing-channel-promise-thenable.js +7-1Lines changed: 7 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -12,6 +12,9 @@ class ResolvedThenable { | 12 | 12 | then(resolve) { | 13 | 13 | return new ResolvedThenable(resolve(this.#result)); | 14 | 14 | } | | 15 | + customMethod() { | | 16 | + return this.#result; | | 17 | + } | 15 | 18 | } | 16 | 19 |
| 17 | 20 | const channel = dc.tracingChannel('test'); | @@ -49,7 +52,10 @@ const result = channel.tracePromise(common.mustCall(function(value) { | 49 | 52 | }), input, thisArg, expectedResult); | 50 | 53 |
| 51 | 54 | assert(result instanceof ResolvedThenable); | 52 | | -assert.notStrictEqual(result, innerThenable); | | 55 | +// With branching then, the original thenable is returned directly so that | | 56 | +// extra methods defined on it remain accessible to the caller. | | 57 | +assert.strictEqual(result, innerThenable); | | 58 | +assert.deepStrictEqual(result.customMethod(), expectedResult); | 53 | 59 | result.then(common.mustCall((value) => { | 54 | 60 | assert.deepStrictEqual(value, expectedResult); | 55 | 61 | })); |
|