Repository navigation
tracingChannel.tracePromise forces native promises #59936
Copy link
Copy link
Closed
Labels
diagnostics_channelIssues and PRs related to the diagnostics_channel module.Issues and PRs related to the diagnostics_channel module.
Description
Version
18.19.0+
Platform
All
Subsystem
diagnostics_channel
What steps will reproduce the bug?
We received a report about an openai method crashing that's been wrapped with tracingChannel.tracePromise. After some digging, I see the issues is here. OpenAI creates a custom promise and it's getting stripped in tracePromise. This is a distilled repro case to show the issue.
import assert from 'node:assert';
import { tracingChannel } from 'node:diagnostics_channel';
const channel = tracingChannel('custom-promise')
channel.subscribe({
asyncStart(data) {
}
})
class CustomPromise {
constructor(executor) {
this.state = 'pending'; // Possible states: 'pending', 'fulfilled', 'rejected'
this.value = undefined; // Will hold the result or error
this.successCallbacks = [];
this.errorCallbacks = [];
// Executor is the function passed to the promise
try {
executor(this._resolve, this._reject);
} catch (error) {
this._reject(error);
}
}
// Custom resolve function
_resolve = (value) => {
if (this.state === 'pending') {
this.state = 'fulfilled';
this.value = value;
this.successCallbacks.forEach(callback => callback(this.value));
}
}
// Custom reject function
_reject = (error) => {
if (this.state === 'pending') {
this.state = 'rejected';
this.value = error;
this.errorCallbacks.forEach(callback => callback(this.value));
}
}
// Then method to handle successful promise resolution
then(successCallback) {
if (this.state === 'fulfilled') {
successCallback(this.value);
} else if (this.state === 'pending') {
this.successCallbacks.push(successCallback);
}
return this; // Allows chaining 🔄
}
// Catch method to handle promise rejection
catch(errorCallback) {
if (this.state === 'rejected') {
errorCallback(this.value);
} else if (this.state === 'pending') {
this.errorCallbacks.push(errorCallback);
}
return this; // Allows chaining 🔄
}
}
function test(arg) {
return new CustomPromise((resolve) => {
setTimeout(() => {
resolve(arg)
}, 100)
})
}
const arg = 'test'
const promise = channel.tracePromise(test, { ctx: true}, this, arg)
assert.equal(promise.constructor.name, 'CustomPromise')
const result = await promise
assert.equal(result, arg)How often does it reproduce? Is there a required condition?
Every time
What is the expected behavior? Why is that the expected behavior?
To properly return the custom promise
What do you see instead?
It returns a native Promise instead
Additional information
I could workaround this by using traceSync and propagating the promise myself, but I'd prefer that the API does the right thing.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
diagnostics_channelIssues and PRs related to the diagnostics_channel module.Issues and PRs related to the diagnostics_channel module.
Activity
Flarna commentedon Sep 21, 2025
This fits the documentation which says Promise Chained from promise returned by the given function
I think for functions which are not fitting into the standard sync/callback/pattern require some hand crafted variant which fits to the lifecycle/api of the concrete object returned.
Renegade334 commentedon Sep 21, 2025
This behaviour is currently conditional on the presence or absence of channel subscribers, and the handling of non-thenables is also variable, so there does at least need to be some formal codification of what the behaviour should be.
Flarna commentedon Sep 22, 2025
I think
traceCallbackhas a similar issue. If the given callback has some extra properties set they are missing on the wrapped callback. if the called API is relying on them it likely results in problems.Also
wrappedCallback.lengthmight differ from original.Likely not that frequently occurring in the wild compared to thenables/custom promises.
Qard commentedon Sep 25, 2025
The native promise upgrade is required for PromisePrototypeThen to work correctly.
We could eliminate the use of the primordial there. I don't recall if the primordials discussion ever came to some clear conclusion, but I know there at least was some discussion that maybe those don't matter?
Flarna commentedon Sep 25, 2025
I think even if we would not use
PromisePrototypeThenthe conversion/assumption for aPromiseis needed/will happen.A
thenableis an object having athen()function. WhilePromise#then()returns aPromisethere is no requirement forThenable#then()to return anything.Renegade334 commentedon Sep 25, 2025
A+ does mandate this, so it's not a thoroughly unreasonable assumption.
Flarna commentedon Sep 25, 2025
For thenables implementing the Promise spec I agree, but not for thenables which do not pretend to be a Promise.
Even our tests have thenables which don't return a Promise.
diagnostics_channel: ensure tracePromise consistency with non-Promises
diagnostics_channel: mark TracingChannel as stable
diagnostics_channel: mark TracingChannel as stable
diagnostics_channel: mark TracingChannel as stable