Make async/await monkey-patchable

[bmeurer]

I'd like to bring up an idea from @gsathya here, which could help to make progress short-/middle-term while we're resolving the performance problems with async_hooks, specifically with promise hooks. As mentioned in nodejs/benchmarking#181 (comment) just enabling async_hooks on a simple hapi v17 server already causes a 30% performance drop, which is way below what's acceptable.

Given the discussions I've had with some many people from the Node community about this since NodeConfEU, I see several ways to make progress on this, but no easy path forward. We might need to make some difficult decisions in what hooks we want to support for promises and the trade-offs for performance we're willing to make. Unfortunately, as pointed out by @ofrobots and others, we're kind of in a hurry now, since async/await is a thing already and monkey-patching doesn't work there because of the way the specification is written. So essentially vendors are forced to use async_hooks now for their monitoring tools, even though it's not ready, and might even undergo breaking changes while it's in EXPERIMENTAL.

So in order to buy us some time, and maybe even enable things like zone.js in the browser, we could instead change the specification to essentially make await monkey-patchable. That means we need to change the places in the specification that allocate the implicit promises for async and await,

and also make await lookup "then" on the promise instead of hardcoding the internal operation.

Both of which sounds somewhat straight-forward, and V8 can still optimize the common case (no prototype mutation) as it does today, i.e. skip the "then" lookup on native promises if we know that no one touched Promise.prototype.then. I think this change could even be back-ported to Node 8. I also think that making such a change wouldn't break web compatibility, although it makes await monkey-patchable in some sense. It would probably still take a while to change the spec, but Node/V8 wouldn't need to wait for that to reach stage 4.

What do you folks think?

cc @nodejs/collaborators @jasnell @ofrobots @littledan @gsathya @caitp @hashseed @AndreasMadsen @mathiasbynens

[Qard]

👍 from me. Jumping that native barrier all the time hurts. Hard.

[vdeturckheim]

As a RASP vendor, this is probably one of the best solution right now!

[mcollina]

How would a vendor monkey-patch async-await to add tracing? My only fear is that if we ask for this it might come back and haunt us for years to come.
What will be the time frame for this to ship?

[bmeurer]

@mcollina I guess it's very similar to how it currently works with promises. You change the Promise property on the global object to be your wrapper, which gives you the init hook and provide a custom "then" function, which wraps the fulfilled and rejected callbacks for the before and after hooks.

My only fear is that if we ask for this it might come back and haunt us for years to come.

It already haunts us with Promise builtins itself, so this just makes it consistent for async/await as far as I can tell. I'm really not excited about monkey-patching in general, but I realize that having this for Promise builtins, but not for async/await, is really odd.

What will be the time frame for this to ship?

As said I guess we could make this change in V8 quickly, maybe couple of days. Getting this back-ported to V8 6.2 (for Node 8) might take another day or two. It also depends on whether this is something we can do in Chrome, or whether we have to flag it for Node initially.

[hashseed]

If it's something we would only offer to Node.js (even if initially), I would add the API to monkey-patch in C++ (i.e. v8.h). This way at least in pure JavaScript Node.js does not diverge from the browser.

[bmeurer]

@hashseed That's kind of exactly what PromiseHooks do, and how we ended up with this problem.
Also it wouldn't solve the problem for the existing monitoring software as far as I understand.

[mcollina]

I am 👍 .

[AndreasMadsen]

@bmeurer 👎 unless you have a good answer for this: how will we implement the destroy hook?

That's kind of exactly what PromiseHooks do, and how we ended up with this problem.

Can you elaborate on why PromiseHooks is the root cause of the problem? Isn't it that the JS-native barrier is expensive, or that the spec doesn't allow monkey-patching.

I think it is important to understand what the problem is before we agree on a solution, even if the solution is temporary. I also think it is important to have a roadmap for a more long-term solution.

Also, it wouldn't solve the problem for the existing monitoring software as far as I understand.

I don't think we should try to solve "the problem for the existing monitoring software", they need to go away from monkey-patching not add more.

---

If async_hooks monkey-patches the Promise instance and there is a good solution to the destroy issue, then I'm okay with this solution but I don't want it to collide with others who monkey-patches Promise.

/cc @nodejs/async_hooks

[bmeurer]

@AndreasMadsen This issue is about keeping existing software working that uses monkey-patching with promises. It's not about replacing async_hooks. Specifically this software already works without destroy hooks.

Can you elaborate on why PromiseHooks is the root cause of the problem? Isn't it that the JS-native barrier is expensive, or that the spec doesn't allow monkey-patching.

There are multiple issues here. The JS-native barrier is definitely the most important one.

I think it is important to understand what the problem is before we agree on a solution, even if the solution is temporary. I also think it is important to have a roadmap for a more long-term solution.

The whole idea of this proposal is to buy us time to think about a long-term solution, while still allowing monitoring software to work properly in a world where async/await is a thing.

If async_hooks monkey-patches the Promise instance, then I'm okay with this solution but I don't want it to collide with others who monkey-patches Promise.

I don't think async_hooks should work by monkey-patching.

[AndreasMadsen]

This issue is about keeping existing software working that uses monkey-patching with promises. It's not about replacing async_hooks.

So it is also not about replacing PromiseHooks?

[bmeurer]

So it is also not about replacing PromiseHooks?

Definitely not. I'm more and more convinced that we should have promise hooks as part of the language specification, because there are similar use cases in the browser. My proposal here is to make sure we don't break existing enterprise software while we try to make promise hooks viable.

[AndreasMadsen]

Definitely not. I'm more and more convinced that we should have promise hooks as part of the language specification, because there are similar use cases in the browser. My proposal here is to make sure we don't break existing enterprise software while we try to make promise hooks viable.

Makes sense. 👍

/cc @JiaLiPassion

[hashseed]

That's kind of exactly what PromiseHooks do, and how we ended up with this problem.
Also it wouldn't solve the problem for the existing monitoring software as far as I understand.

I think you misunderstand. What I'm suggesting is that you would only expose the monkey-patchability in V8's C++ API, not in JavaScript. I.e to monkey patch, you would need to use the C++ API. You would still monkey-patch with a JavaScript function, not a C++ callback.

[vdeturckheim]

@hashseed I'm not certain I understand, would that require a C++ addons in APM/RASP agents?

[hashseed]

What I'm suggesting is that V8 does not expose this monkey-patching feature in JavaScript, but offer a C++ API to do so. Node.js can then expose that C++ API to JS in, say, the vm module.

[vdeturckheim]

Okay, thanks for the clarification!

[bmeurer]

@hashseed The existing software already monkey-patches via

global.Promise = MyPromise;

or

Promise.prototype.then = function() { ... }

so all we need to do is to use that in async/await like we do in Promise builtins. We don't need any new APIs for that.

[hashseed]

That's not the issue. The issue with just "use that" is that this would be unspec'ed behavior.

[bmeurer]

Sure, but that's the same with async_hooks, which also make await call into arbitrary JavaScript. Also note that I propose to adapt the specification for await and async function to make this possible.

[hashseed]

All I'm asking is not for V8 to expose this via JS directly as long as it's not part of the spec. Node would be free to expose it via internal modules.

Unless you want to run into the situation where pure JS code that is seemingly spec conform only works on Node but not Chrome. Also there is no guarantee it will be accepted at TC39.

It's fairly obvious that async hooks are not standard JS and there is no risk of people accidentally trying to make async hook work on Chrome.

[bmeurer]

@hashseed Ok, so we should first figure out what's the state regarding the spec?

[hashseed]

How about introducing a v8::Promise::SetGlobalConstructor or something similar that overrides both the global Promise and whatever async/await uses, but ovewriting the global Promise in JS does not change? That way Node.js can do what you propose, until we brought into the spec, and browser behavior is not compromised.

[hashseed]

Also sync up with our language team upfront :)

[bmeurer]

I see what you're saying. Yeah that'd do the trick I guess.

[domenic]

I've been asked to comment here on whether changing the spec is a good idea. Personally I don't think so. We have a long history of syntax being "trustworthy" and not monkey-patchable, e.g. object literal properties not invoking setters on Object.prototype, array/object literal syntax not invoking any monkey-patched Object/Array constructor, and so on. The intent of the async/await spec was to provide similar guarantees, that you'd be guaranteeing no interference between you and the native Promise constructor.

I've favored for a long time making all async operations trackable (but not modifiable) via zones, instead of introducing a one-off at the language level for async/await.

I think if Node.js/V8 wants to introduce some specific "promise hooks v2" that works similar to what @hashseed describes, that seems fine, as a way of saying that this particular environment has decided to trade the trustworthiness guarantees of the standard for other capabilities. But I'd rather not change the language spec to allow arbitrary monkeypatching.

[bmeck]

@bmeurer do you think Zones would be optimizable in the same way as what is proposed above?

[bmeurer]

@bmeck I've been told that you only need to bridge the current gap for await in order to successfully implement Zones via monkey-patching. Which is a pro, since that means it'll work the same in the browser and Node (of course a spec'd PromiseHooks API would probably be superior to that). Not sure what you mean by optimizable.

@domenic Thanks for your feedback. I share your sentiment that syntax should be trustworthy in general and I'm also not a big fan of monkey-patching usually. I do however also buy the argument about consistency here.

[bmeurer]

Ok, so summarizing the discussion here and the discussions I've had with JavaScript folks about this via other channels (especially @ajklein and @domenic), the conclusion is that we cannot just violate the specification here, so essentially we'd need to do exactly what @hashseed suggested and enable the behavior via a special C++ API call. But that also means that existing monkey-patching approaches will still not work out of the box with async/await. In that light it probably doesn't matter for monitoring software vendors whether they depend on some spec violating C++ API or an experimental async_hooks API for their products.

Potential steps forward:

1. Look into spec'ing promise hooks.
2. Continue investigating current async_hooks performance.

I hope that makes sense, and thanks to everyone who helped to enlighten me on this topic.

[bmeurer]

Let's follow up on PromiseHooks performance in #144

[littledan]

Chiming in late here, but the conclusion seems reasonable to me. It was a deliberate design decision of TC39 to not allow monkey patching here, and I wouldn't expect it to be reversed lightly. It may also be complex to make the monkey-patched Promise methods interact well with devtools (something transpiled async/await simply didn't try to do).

[hashseed]

It may also be complex to make the monkey-patched Promise methods interact well with devtools

A very good point. Let's make exception prediction not even more complicated :D