Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AsyncResource.bind does not forward arguments #36051

Closed
lroal opened this issue Nov 9, 2020 · 7 comments
Closed

AsyncResource.bind does not forward arguments #36051

lroal opened this issue Nov 9, 2020 · 7 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations.

Comments

@lroal
Copy link

lroal commented Nov 9, 2020

Using Node 12.19.0:
When binding a function with AsyncResource.bind, the arguments are not forwarded.
I had to write this workaround to make it work:

function bindWorkAround(cb) {
	AsyncResource.bind(invokeOriginal);
	let _arguments;
	return onData;

	function onData() {
		_arguments = arguments;
		invokeOriginal();
	}

	function invokeOriginal() {
		cb.apply(null, _arguments);
	}

}
@mmomtchev
Copy link
Contributor

@lroal, can you please edit your issue to include the code that does not work and describe what you expect to happen? You can also include the workaround, but what is the most important is the code that does not work.

@lroal
Copy link
Author

lroal commented Nov 9, 2020

Here is the example, @mmomtchev .
I expect it to print 'foo' instead of undefined.

let {AsyncResource} = require('async_hooks');

let cb = AsyncResource.bind(onData);
cb('foo');

function onData(arg) {
	console.log(arg); //undefined
}

@Flarna
Copy link
Member

Flarna commented Nov 9, 2020

The first argument passed to the bound function is this in the called function.
Try the following:

let {AsyncResource} = require('async_hooks');

function onData(arg) {
	console.log(this, arg); // foo bar
}

let cb = AsyncResource.bind(onData);
cb('foo', 'bar');

@targos targos added the doc Issues and PRs related to the documentations. label Nov 10, 2020
@targos
Copy link
Member

targos commented Nov 10, 2020

Then that's a documentation issue

@Flarna Flarna added the async_hooks Issues and PRs related to the async hooks subsystem. label Nov 10, 2020
@lroal
Copy link
Author

lroal commented Nov 10, 2020

The first argument passed to the bound function is this in the called function.
Try the following:

let {AsyncResource} = require('async_hooks');

function onData(arg) {
	console.log(this, arg); // foo bar
}

let cb = AsyncResource.bind(onData);
cb('foo', 'bar');

It prints: [String: 'foo'] bar

The behaviour is very surprising. Why do we need to involve the "this" key word ?
You would expect the bind method too act as as simple proxy for callback. Not intertwined with "this".
I think it would be better if the method is consistent with signature of domain.bind . The intention is that async_hooks should replace the domain - so staying close to the orignal api is a good idea.

@Flarna
Copy link
Member

Flarna commented Nov 10, 2020

The API is just a wrapper around asyncResource.runInAsyncScope which allows to pass this similar as Reflect.apply().
Allowing user to specify the target is helpful to use it with class methods.

But it seems reusing the name bind causes confusions as one could assume that it acts similar as Function.bind().

@jasnell as author of #34574 any thoughts regarding this?

@jasnell
Copy link
Member

jasnell commented Nov 10, 2020

Definitely a documentation issue.

So, the correct use of the function is:

const ar = new AsyncResource('foo')

const fn = ar.bind((...args) => console.log(this, args));
fn({}, 1, 2, 3); // where the {} is thisArg

We could modify the API to accept the additional this argument to help avoid the confusion.

const ar = new AsyncResource('foo')

const fn = ar.bind((...args) => console.log(this, args), {}); // pass the additional this arg
fn(1, 2, 3);

Would make for a good first contribution to do both the doc update (to add an example of correct usage) and add the additional argument.

jasnell added a commit to jasnell/node that referenced this issue Jan 6, 2021
Semver-major

Support setting the `thisArg` for AsyncResource.bind and
AsyncResource.prototype.bind. If `thisArg` is not set,
then `this` will be set to the `AsyncResource` instance.

Fixes: nodejs#36051
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell closed this as completed in 324a6c2 Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants