Repository navigation
AsyncResource.bind always alters the value of this unexpectedly #42158
Description
Activity
thisArg = thisseems incorrect, that would bind it to the AsyncResource, which is what you're seeing. /cc @nodejs/async_hooksI agree that this is not nice and would prefer to have it different. But I guess a change would be semver major as
AsyncResourceis no longer experimental.Reacted by Andrei Pechkurov and Benjamin GruenbaumI've been trying to understand the reasoning behind this, and the only thing I can think of is that for the static
AsyncResource.bindyou would otherwise have no way to get a reference to theAsyncResourceinstance that is created, so maybe there could be an argument for the static method. However, for theAsyncResource.prototype.bindinstance 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 thethisArganyway 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_hooksitself) 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
thiswhich could negatively impact the code ifthisis used in the function. In any case, forAsyncResourceto 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
thisby default makes the most sense.for the static
AsyncResource.bindyou would otherwise have no way to get a reference to theAsyncResourceinstance that is createdThe static
bind(and also the method) return a function which has aasyncResourceproperty pointing to the created resource.The current doc doesn't state that
thisis bound to theAsyncResourceinstance in case user doesn't provide a specific value for this. Not sure if this is enough to judge this as semver patch.@Flarna I'm not sure when you want to ignore the
thisvalue 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 thethisand doesn't let it be done dynamically later.domain.bind()doesn't do the forced setting ofthiseither.Reacted by Roch DevostI have also no use case in mind and for me patch/bug is fine.
Version
17.6.0
Platform
No response
Subsystem
No response
What steps will reproduce the bug?
How often does it reproduce? Is there a required condition?
Always as the code is explicitly binding
thisArgeven when not provided.What is the expected behavior?
AsyncResource.bindshould use thethisvalue provided by the caller by default and only bindthiswhen athisArgis passed explicitly.What do you see instead?
AsyncResource.binddefaults to settingthisto itself, which is not only unexpected in pretty much all cases, but also makes it impossible to use thethisprovided 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 userunInAsyncScopeinstead ofbindmaking 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