perf_hooks: fix timerify bug

[iam-frankqiu]

Fixes: #42743

[himself65]

I've fixed the issue

[nodejs-github-bot]

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

[himself65]

could you please add a test case for this?

[hax]

Here we retrieve result.then twice which theoretically not work like JS spec strictly. We'd better write a helper to ensure such subtle semantic. (Not sure whether Node.js already have such internal thenable helpers).

[himself65]

What do you mean?

[himself65]

Do you mean like this?

node/lib/assert.js

Lines 763 to 771 in 1e76165

	function checkIsPromise(obj) {
	// Accept native ES6 promises and promises that are implemented in a similar
	// way. Do not accept thenables that use a function as `obj` and that have no
	// `catch` handler.
	return isPromise(obj) ||
	(obj !== null && typeof obj === 'object' &&
	typeof obj.then === 'function' &&
	typeof obj.catch === 'function');
	}

[hax]

I mean, if follow JS spec strictly, the code should be:

const {then} = result
if (typeof then === 'function') {
  ReflectApply(then, result, [callback])
}

Because theoretically result.then could be a getter, and two retrieve could give u different result.

[hax]

I also don't understand why checkIsPromise "do not accept thenables that use a function as obj and that have no catch handler" intentionally, this limitation seems wrong to me.

[himself65]

also, we need to edit the document

node/doc/api/perf_hooks.md

Lines 336 to 338 in 7a53696

	If the wrapped function returns a promise, a finally handler will be attached
	to the promise and the duration will be reported once the finally handler is
	invoked.

[hax]

One idea which is not relate to the bug directly, but:

currently, now is called in the processComplete (duration = now() - start) , but consider the goal is to calc the time as precise as possible, it may be better to calc it outside like:

const t = now()
processComplete(fn.name, start - t, args, histogram)

(also need to modify other related places, omit here)

This could rule out the invoking time of processComplete, in the cases of microbenchmark, such cost can't be ignored.