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 always alters the value of this unexpectedly #42158

Closed
rochdev opened this issue Mar 1, 2022 · 6 comments · Fixed by #42177
Closed

AsyncResource.bind always alters the value of this unexpectedly #42158

rochdev opened this issue Mar 1, 2022 · 6 comments · Fixed by #42177
Labels
async_hooks Issues and PRs related to the async hooks subsystem.

Comments

@rochdev
Copy link
Contributor

rochdev commented Mar 1, 2022

Version

17.6.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

const fn = AsyncResource.bind(function () {
  console.log(this)
})

fn.apply('foo') // will log the AsyncResource instead of 'foo'

const obj = { fn }

obj.fn() // will log the AsyncResource instead of `obj`

How often does it reproduce? Is there a required condition?

Always as the code is explicitly binding thisArg even when not provided.

What is the expected behavior?

AsyncResource.bind should use the this value provided by the caller by default and only bind this when a thisArg is passed explicitly.

What do you see instead?

AsyncResource.bind defaults to setting this to itself, which is not only unexpected in pretty much all cases, but also makes it impossible to use the this provided by the caller without an explicit reference to the caller at bind time. This means that in all situations right now it would make more sense to use runInAsyncScope instead of bind making it effectively unusable.

Additional information

As an APM vendor, we're making heavy use of this API and there is not a single occurrence in our code where the default works.

The feature was originally implemented in #36782

@devsnek
Copy link
Member

devsnek commented Mar 1, 2022

thisArg = this seems incorrect, that would bind it to the AsyncResource, which is what you're seeing. /cc @nodejs/async_hooks

@devsnek devsnek added the async_hooks Issues and PRs related to the async hooks subsystem. label Mar 1, 2022
@Flarna
Copy link
Member

Flarna commented Mar 1, 2022

I agree that this is not nice and would prefer to have it different. But I guess a change would be semver major as AsyncResource is no longer experimental.

@rochdev
Copy link
Contributor Author

rochdev commented Mar 1, 2022

I've been trying to understand the reasoning behind this, and the only thing I can think of is that for the static AsyncResource.bind you would otherwise have no way to get a reference to the AsyncResource instance that is created, so maybe there could be an argument for the static method. However, for the AsyncResource.prototype.bind instance method I can't find a single use case where this would make sense as you already have a reference to the instance which you could just pass as the thisArg anyway if needed. I would also argue that if you do need the instance all you would have to do is to use an actual instance instead of the static method, so I'd probably keep it consistent in both cases to avoid confusion.

Maybe @jasnell can provide the reasoning behind the current implementation, but right now I can't really think of a single use case (outside of maybe Node core and async_hooks itself) where the current implementation would not be confusing at best and cause issues at worst.

As for semver, I think it depends if this is bad enough to warrant making it a bug fix since it overrides the value of this which could negatively impact the code if this is used in the function. In any case, for AsyncResource to be used in libraries right now it would probably rely on a polyfill anyway to support older Node versions, so it should be fine to put the change in a major.

In any case, I can work on a PR if everyone agrees that not binding this by default makes the most sense.

@Flarna
Copy link
Member

Flarna commented Mar 1, 2022

for the static AsyncResource.bind you would otherwise have no way to get a reference to the AsyncResource instance that is created

The static bind (and also the method) return a function which has a asyncResource property pointing to the created resource.

The current doc doesn't state that this is bound to the AsyncResource instance in case user doesn't provide a specific value for this. Not sure if this is enough to judge this as semver patch.

@bmeck
Copy link
Member

bmeck commented Mar 3, 2022

@Flarna I'm not sure when you want to ignore the this value of a caller in JS which makes me think it is a patch / bug and undocumented. The only thing I can imagine is comparison with function.prototype.bind which sets the this and doesn't let it be done dynamically later. domain.bind() doesn't do the forced setting of this either.

@Flarna
Copy link
Member

Flarna commented Mar 3, 2022

I have also no use case in mind and for me patch/bug is fine.

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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants