`AsyncResource.bind` always alters the value of `this` unexpectedly

[rochdev]

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]

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

[Flarna]

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]

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]

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]

@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]

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