tracingChannel.tracePromise forces native promises

[bizob2828]

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.

[Flarna]

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]

This fits the documentation which says Promise Chained from promise returned by the given function

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]

I think traceCallback has 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.length might differ from original.

Likely not that frequently occurring in the wild compared to thenables/custom promises.

[Qard]

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]

I think even if we would not use PromisePrototypeThen the conversion/assumption for a Promise is needed/will happen.
A thenable is an object having a then() function. While Promise#then() returns a Promise there is no requirement for Thenable#then() to return anything.

[Renegade334]

A thenable is an object having a then() function. While Promise#then() returns a Promise there is no requirement for Thenable#then() to return anything.

A+ does mandate this, so it's not a thoroughly unreasonable assumption.

[Flarna]

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.