async_hooks: add bindToAsyncScope method to AsyncResource

[puzpuzpuz]

Closes #33723

Adds bindToAsyncScope() method to AsyncResource class. This method should be handy for AsyncLocalStorage users (and not only them). Say, it allows binding EE/stream listeners to the specific resource:

const { createServer } = require('http');
const { AsyncResource, executionAsyncId } = require('async_hooks');

const server = createServer(function(req, res) {
  const asyncResource = new AsyncResource('request');
  const asyncId = asyncResource.asyncId();
  // The listener will always run in the execution context of `asyncResource`.
  req.on('close', asyncResource.bindToAsyncScope(() => {
    console.log(executionAsyncId()); // Prints the value of `asyncId`.
  }));
  res.end();
}).listen(3000);

cc @nodejs/async_hooks

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

What's advantage over?

asyncResource.runInAsyncScope.bind

I guess it saves a some duplication (i.e. don't have to type asyncResource 2 more times)... but I'm not sure it's worth it... adds more API surface and possible confusion to what it actually does?

[puzpuzpuz]

What's advantage over?

asyncResource.runInAsyncScope.bind

I guess it saves a some duplication (i.e. don't have to type asyncResource 2 more times)... but I'm not sure it's worth it... adds more API surface and possible confusion to what it actually does?

@ronag you're probably right. The main advantage is briefer code. Domains and user-land modules have similar .bind() methods, so I thought it'll be convenient for users to have something similar in async_hooks (AsyncResource).

If there will be no voices to have this new method, I'm willing to close this PR.

[BridgeAR]

If we do not implement this as separate API, I strongly suggest to document asyncResource.runInAsyncScope.bind with an example. It will otherwise likely frequently happen that people ask for such feature / try to find a solution and fail.

We have a similar situation with util.promisify() where we did not implement a bind option because it's already possible to do the same with the language and people asked for such a feature multiple times.

[Flarna]

The .length property of the returned function is always 0 here. This may cause problems if the retruned function is used in frameworks like express which use fn.length to decide which type of handler it is (https://github.com/expressjs/express/blob/master/lib/router/layer.js).

Also properties like Symbol(customPromisifyArgs) are stripped which may cause problems.

Both issues can be solved by caller but maybe we should at least document it?

[targos]

We can also set name and length to meaningful values before returning the function

[puzpuzpuz]

Good catch! I'll make sure to address it, if this PR won't be closed.

[ronag]

This is a little magical though... documenting it would be my preference

[Flarna]

length, name and Symbol(customPromisifyArgs) are quite easy and most likely harmless if set on wrapped function. The hard part are other properties as they may have special meaning where copy is simply wrong - and not copying either.

[puzpuzpuz]

@ronag could you elaborate on how the snippet from the PR description would look like with asyncResource.runInAsyncScope.bind? After some thinking I can only imagine alternatives with non-elegant code. Function.bind itself seems to be affecting this keyword evaluation. Am I missing something?

[ronag]

@ronag could you elaborate on how the snippet from the PR description would look like with asyncResource.runInAsyncScope.bind?

  req.on('close', asyncResource.runInAsyncScope.bind(asyncResource, () => {
    console.log(executionAsyncId()); // Prints the value of `asyncId`.
  }));

[puzpuzpuz]

  req.on('close', asyncResource.runInAsyncScope.bind(asyncResource, () => {
    console.log(executionAsyncId()); // Prints the value of `asyncId`.
  }));

@ronag thanks. This snippet seems to be working. Yet, it has one problem: there should be the third argument, as asyncResource.runInAsyncScope() expects thisArg as the second argument. Here is an illustration:

const a = new AsyncResource('foobar');

function foo(bar) {
  assert.strictEqual(executionAsyncId(), a.asyncId());
  return bar;
}

const ret1 = a.runInAsyncScope.bind(a, foo)('baz');
assert.strictEqual(ret1, 'baz'); // fails

// this one works
const ret2 = a.runInAsyncScope.bind(a, foo, undefined)('baz');
assert.strictEqual(ret2, 'baz'); // passes

[ronag]

expects thisArg as the second argument.

I'm aware of this. Since you were using an arrow function in the example it was not required.

[puzpuzpuz]

I'm aware of this. Since you were using an arrow function in the example it was not required.

Makes sense. I'd say that it has to do more with the fact that the snippet contains a no-args function. An arrow function that expects arguments still requires an extra argument in the Function.bind() call:

const ret = a.runInAsyncScope.bind(a, (bar) => {
  assert.strictEqual(executionAsyncId(), a.asyncId());
  return bar;
})('baz');
assert.strictEqual(ret, 'baz'); // fails

Nevertheless, I'm still not sure if this PR brings any value in the light of asyncResource.runInAsyncScope.bind. Does it make sense to close it and submit another one that would add a doc sample with EE + asyncResource.runInAsyncScope.bind?

[benjamingr]

We have a similar situation with util.promisify() where we did not implement a bind option because it's already possible to do the same with the language and people asked for such a feature multiple times.

Fwiw that's not entirely comparable, util.promisify was added following an already existing ecosystem of libraries (Bluebird's promisify and Q's nfcall) that supported this feature.

Other than that - I agree with the conclusion and a doc-fix.

(I still think it would be very beneficial to provide a one-stop shop to converting an API to promises - though it's less crucial now that the ecosystem has largely migrated to promises already)

[puzpuzpuz]

Closing this one in favor of upcoming doc enhancement PR.

[puzpuzpuz]

Created the doc enhancement PR: #33751