diagnostics_channel: ensure tracePromise consistency with non-Promises

[Renegade334]

tracingChannel.tracePromise() is documented in the source code (albeit not in the docs) to accept functions that return non-Promise thenables. However, it currently has some wonky behaviour when the return value isn't a native Promise, and also in the undocumented case where the return value isn't promise-like at all.

What gets returned by tracePromise() is highly dependent on whether the tracing channel has active subscribers or not:

result type	Without subscribers		With subscribers
	Return value	Return type	Return value	Return type
Native Promise	result	Native Promise	result.then(...)	Native Promise
Custom thenable	result	Custom thenable	Promise.resolve(result).then(...)	Native Promise
Non-thenable	result	Non-thenable	Promise.resolve(result).then(...)	Native Promise

The major inconsistencies are:

• If the wrapped function returns a custom thenable, then it will either be returned as a custom thenable or wrapped in a native Promise, depending on whether there are active subscribers.
• If the wrapped function returns a non-thenable, then it will either be returned verbatim or wrapped in a resolved native Promise, depending on whether there are active subscribers.
• In the case where the wrapped function synchronously returns a non-thenable, then tracePromise produces asyncStart/asyncEnd events, despite no asynchronous execution. On the other hand, if the wrapped function throws synchronously, these events are not produced.

This PR makes some changes to the non-Promise cases to make the behaviours consistent.

• If the wrapped function returns either a Promise or a custom thenable, then the value returned by tracePromise will have the same type: either the original value (if no subscribers) or the value of result.then(...) (if active subscribers).
• If the wrapped function returns a non-thenable result, then it is returned verbatim, and a warning is emitted. No asyncStart or asyncEnd events are produced.

result type	Without subscribers		With subscribers
	Return value	Return type	Return value	Return type
Native Promise	result	Native Promise	result.then(...)	Native Promise
Custom thenable	result	Custom thenable	result.then(...)	Custom thenable
Non-thenable	result	Non-thenable	result	Non-thenable

Fixes: #59936

[Renegade334]

Throwing here avoids Promisifying thenables on rejection. The Promises/A+ standard dictates that throwing within the promise handlers results in the returned thenable rejecting. (https://promisesaplus.com/#point-42)

For native Promises, there should be no observable change.

[Flarna]

Is there already an existing testcase for this?
if not maybe a good idea to add one.

[Renegade334]

It's covered by

node/test/parallel/test-diagnostics-channel-tracing-channel-promise-error.js

Lines 30 to 36 in 3bbed16

	assert.rejects(
	channel.tracePromise(common.mustCall(function(value) {
	assert.deepStrictEqual(this, thisArg);
	return Promise.reject(value);
	}), input, thisArg, expectedError),
	expectedError,
	).then(common.mustCall());

[codecov]

Codecov Report

❌ Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.71%. Comparing base (9cc7fcc) to head (1c9c33b).
⚠️ Report is 168 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/diagnostics_channel.js	84.21%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61766      +/-   ##
==========================================
- Coverage   89.72%   89.71%   -0.01%     
==========================================
  Files         675      675              
  Lines      204797   204805       +8     
  Branches    39344    39346       +2     
==========================================
- Hits       183752   183744       -8     
- Misses      13324    13334      +10     
- Partials     7721     7727       +6     

Files with missing lines	Coverage Δ
lib/diagnostics_channel.js	98.45% <84.21%> (-0.65%)	⬇️

... and 31 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.

[Renegade334]

@nodejs/diagnostics any eyes welcome.

[Flarna]

Is there already an existing testcase for this?
if not maybe a good idea to add one.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71689/