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.