diagnostics_channel: ensure tracePromise return values are unmodified

[Renegade334]

There are some behavioural issues with tracingChannel.tracePromise():

• This method is the only tracing method which alters the return value of the wrapped function. traceSync() and traceCallback() pass their results through unchanged, whereas tracePromise() returns a newly constructed Promise. The original return value is never accessible to the user, which can cause issues if – say – the function originally returned a custom promise with additional methods/properties that the user needs to access.
• Synchronously returned non-thenables are wrapped in Promises and trigger async tracing events, whereas synchronously thrown exceptions aren't and don't.
• These behaviours are not even predictable, as they depend on the subscriber state of the tracing channel. If no subscribers are registered, tracePromise() currently behaves just like traceSync(), and passes through the function return value verbatim; whereas if any are registered, then it wraps and returns a new Promise. This is especially noticeable for non-promise return values: tracePromise(() => 42) might return 42 or Promise.resolve(42), depending on the state of the tracing channel.

This change addresses these issues.

• If a non-thenable value is returned by the function, or it throws synchronously, then these are passed straight through to the user. Async tracing events are not emitted.
• If a promise or thenable is returned by the function, the resolve handlers are registered on the promise, and then the original promise is returned to the user. Async tracing events are emitted when the promise resolves, as before.
• This means that tracePromise() now always returns the original function return value, and removes the unintentional state-dependent polymorphism.

This does change the design of the API, so this is something of a request for comment. Would be a semver-major PRs that contain breaking changes and should be released in the next major version. change.

Fixes: #59936

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.24%. Comparing base (4612c79) to head (bf941af).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59944      +/-   ##
==========================================
- Coverage   88.25%   88.24%   -0.02%     
==========================================
  Files         703      703              
  Lines      207412   207416       +4     
  Branches    39893    39890       -3     
==========================================
- Hits       183052   183031      -21     
- Misses      16313    16354      +41     
+ Partials     8047     8031      -16     

Files with missing lines	Coverage Δ
lib/diagnostics_channel.js	98.88% <100.00%> (-0.22%)	⬇️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Flarna]

doesn't this swallow a reject in case the caller never registers a than nor a catch handler?

[Renegade334]

It does not. The returned promise isn't the one returned by .then(), but the original promise, which remains rejected regardless of whether any reject callbacks have been called.

The promise returned by .then() would indeed never reject, but this is just discarded.

[Flarna]

Did a fast try. Following test is green now but fails with your change:

'use strict';

const common = require('../common');
const dc = require('diagnostics_channel');
const assert = require('assert');

const channel = dc.tracingChannel('test');

const expectedError = new Error('test');
const input = { foo: 'bar' };
const thisArg = { baz: 'buz' };

process.on('unhandledRejection', common.mustCall((reason) => {
  assert.deepStrictEqual(reason, expectedError);
}));

function check(found) {
  assert.deepStrictEqual(found, input);
}

const handlers = {
  start: common.mustCall(check),
  end: common.mustCall(check),
  asyncStart: common.mustCall(check),
  asyncEnd: common.mustCall(check),
  error: common.mustCall((found) => {
    check(found);
    assert.deepStrictEqual(found.error, expectedError);
  })
};

channel.subscribe(handlers);

// Set no then/catch handler to verify unhandledRejection event
channel.tracePromise(function(value) {
  assert.deepStrictEqual(this, thisArg);
  return Promise.reject(value);
}, input, thisArg, expectedError);

[Renegade334]

Unhandled rejection errors would indeed be suppressed by this approach.

[bizob2828]

This does change the design of the API, so this is something of a request for comment. Would be a semver-major change.

@Renegade334 I'm not familiar with the rules of deciding where to land this but since this API is experimental would it really need to be in a semver major?

[Renegade334]

For a significant change in return behaviour, this probably does still warrant a major change.

I'll mock up a new approach later allowing returned chained thenables, as discussed in the associated issue.