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

AsyncContext.{Snapshot,Variable}.prototype.run leave no room to specify receiver #80

Open
gibson042 opened this issue Apr 11, 2024 · 14 comments

Comments

@gibson042
Copy link

(originally posted by @gibson042 in tc39/proposal-promise-try#15 (comment))

I have reservations about Promise.try AsyncContext.{Snapshot,Variable}.prototype.run accepting variadic arguments that leave no room for ever specifying a receiver—setting it apart from Function.prototype.{apply,call} and Reflect.apply.

@bakkot
Copy link

bakkot commented Apr 11, 2024

Can you say more about what exactly those reservations are? What use case are you worried about ruling out here?

@gibson042
Copy link
Author

Well, it is conceivable that the future evolution of the language will lead to a place where heavy use of receivers is commonplace. The existing primary indirect invocation facilities—Function.prototype.apply, Function.prototype.call, and Reflect.apply—all provide a way to specify receiver, and for that matter so do others such as callback-accepting Array.prototype methods (every, filter, map, etc.). They are therefore prepared for such a future, but the run methods proposed here are not and never can be because there's nowhere to put a thisArg parameter... authors would be forced to instead jump through hoops like asyncSnapshot.run((...args) => Reflect.apply($callback, $receiver, args)). So we've got 1) extra cognitive burden to track which argument-propagating facilities also propagate a receiver and which do not, and 2) missing functionality.

From my perspective, the correct framing is not "what is lost by omitting a thisArg parameter?", but rather "what benefits justify its omission as something other than a gratuitous deviation?".

@jridgewell
Copy link
Member

A few points:

  • Ecosystem precedent, Node's AsyncLocalStorage.p.run similarly takes variadic args without a thisArg.
  • I have never once used an Array method thisArg, and they're not variadics.
  • .apply()/.call() aren't run-of-the-mill functions, they're the special functions that expose a low-level ability to devs.

I don't think it's common to need to change the context, and when an object's context needs to be carried over to a method I'd much rather to see a dev write run(() => obj.method()) than run(obj.method, obj).

@Qard
Copy link

Qard commented Apr 12, 2024

AsyncLocalStorage not taking a receiver is generally considered to be a mistake, which is why other related things like diagnostics_channel do take receivers.

@jridgewell
Copy link
Member

Is there a thread on that?

@jridgewell
Copy link
Member

We discussed this in today's meeting. No one voiced support for adding thisArg to run() methods, and if we get pushback from the committee we'd rather remove the variadic args entirely than add it.

@Qard
Copy link

Qard commented Apr 17, 2024

So the main reason for it is to enable store.run(...) to universally be able to replace a call without additional closures. For example, foo(bar, bar) can directly become store.run(context, foo, bar, bar). With instance methods we need to do something like store.run(context, instance.method.bind(instance), ...) which is a bit cumbersome but honestly not a huge deal.

The one downside to not doing it though is that optimizations could likely be made to combine the call/apply that needs to happen with the context change. Could also probably do some magic to avoid dealing with iterators since arguments are passed positionally, just offset by two from the context object and function reference--might be able to do something like forwarding an offset pointer as args into the given function or things like that.

My thinking with context runs has always been to treat it like a special case of function call/apply.

@jridgewell
Copy link
Member

In cases where code wants to invoke a method on an object, I'd much rather see run(()=>foo.bar()) than run(foo.bar, foo). And in cases where you're changing the context dynamically, I'd again much rather see run(() => bar.call(foo)).

I was the only one supportive of using variadic arg style for run, because I practice static functions as my default coding style. Everyone else just said to use an arrow closure.

@bakkot
Copy link

bakkot commented Apr 17, 2024

If this does come up, I'd prefer to address it by adding a runMethod(foo.bar, foo, ...args) helper rather than by changing the signature of run.

@andreubotella
Copy link
Member

andreubotella commented Apr 17, 2024

My thinking with context runs has always been to treat it like a special case of function call/apply.

For what's worth, my feeling is that most non-APM uses of AsyncContext don't see run as a special case of function call/apply by any means. They'll use run with an arrow function, seeing it as a logical part of the caller function, and would prefer to use something like a disposable scope instead, which is why #60 is a topic of discussion.

@Qard
Copy link

Qard commented Apr 18, 2024

Yes, it is the case that often closures will be used by users, but they're also expensive if the run happens in a hot-path so I think we should consider offering tools that are at least capable of avoiding closure use.

Having separate methods is perfectly reasonable to me, so long as the capability exists and we have an interface which can be optimized well for those that care about performance.

@jridgewell
Copy link
Member

jridgewell commented Apr 19, 2024

Some discussion happened in Matrix yesterday. The other delegates agree with my opinions that supporting thisArg ins't desirable. A few comments to highlight:

maybe because most of the time you don't need to specify receiver, and when you do, you have call-bound call to bail you out? – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L23

I think "call-bound call" (const call = Function.call.bind(Function.call); call(obj.method, obj, arg1, arg2)) is a little too much setup to manually, but if we get a demethodize or static helper equivalent then this could be solved orthogonally:

// Imagine this already exists
Reflect.call = Function.call.bind(Function.call);

snapshot.run(Reflect.call, obj.method, obj, arg1, arg2);

Function.prototype.apply/call and Reflect.apply are special helpers, which are very rarely sensible precedent to use for other methods – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L24

Very much agree.

+1 to this, especially because of language learnability. this/receivers aren't things that come early when learning JS, especially because of how they differ from the equivalents in other languages. Even if Promise.try and AsyncContext are advanced use cases, language learning is non-linear. – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L39

This is a good point that I hadn't fully considered. I like seeing () => obj.method() because it's easier for me to understand, but this might be really important for beginners so that they don't get confused about this binding.

@Qard
Copy link

Qard commented Apr 20, 2024

Passing in Reflect.call seems reasonable to me, so long as we keep the argument pass-through so we can avoid a closure wrap or a bind-and-then-call when we care about performance.

My primary concern is that run be fast enough that we can use it liberally. Us APM vendors are going to be calling this all the time to store our current spans and other observability data so performance is absolutely critical to us.

Random side thought: could a decorator transform any form of existing call (bare function, method call, etc.) in a way that V8 could apply the context around it without additional function bind/call/apply? That could be another interesting way to wrap an existing call in a context with minimal extra cost. 🤔

@gibson042
Copy link
Author

Function.prototype.apply/call and Reflect.apply are special helpers, which are very rarely sensible precedent to use for other methods – https://matrixlogs.bakkot.com/TC39_Delegates/2024-04-18#L24

For the record, color me convinced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants