deps: cherry-pick ca76392 from upstream V8

[BridgeAR]

Original commit message:

Correctly run before/after hooks for await.

This fixes a bug where we didn't run before/after hooks for await when
the debugger is not active, as reported downstream in
https://github.com/nodejs/node/issues/20274

Change-Id: I1948d1884c591418d87ffd1d0ccb2bebf4e908f1
Reviewed-on: https://chromium-review.googlesource.com/1039386
Commit-Queue: Benedikt Meurer <[email protected]>
Reviewed-by: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#52909}

Refs: v8/v8@ca76392
Fixes: #20274

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/14625/
V8 CI https://ci.nodejs.org/job/node-test-commit-v8-linux/1352/

/cc @nodejs/v8-update

[MylesBorins]

@BridgeAR @bmeurer should we try and get this merged on 6.6 or 6.7 rather than cherry-picking?

[BridgeAR]

@MylesBorins @bmeurer said Don't think we'll back-merge to 6.6 or 6.7, since this doesn't affect Chrome.

That is why I cherry-picked it.

[devsnek]

we could still ask for it for node's sake

[BridgeAR]

I added a regression test.

New CI https://ci.nodejs.org/job/node-test-pull-request/14641/

[ofrobots]

I fear these precise numbers would cause brittleness going forward. The relationships between the numbers matters but not their precise values.

[bmeurer]

@MylesBorins According to our policy we won't backport this to 6.6 and 6.7. You'll have to check with @natorion directly.

[apapirovski]

Rubberstamp for the patch itself but I would prefer it the test conformed to the majority of our async hook tests. There are lots of utilities that should help with writing robust tests that don't rely on pushing the exact IDs into an arbitrary array.

[natorion]

merging this to 6.7 should be fine because it does not affect Chrome. Please open a merge request.

[MylesBorins]

Bug to track backport to 6.7: https://bugs.chromium.org/p/v8/issues/detail?id=7720
CL: https://chromium-review.googlesource.com/c/v8/v8/+/1042798

[mcollina]

This PR does not fix the following:

'use strict'

const http = require('http')
const sleep = require('util').promisify(setTimeout)
const asyncHooks = require('async_hooks')
const asyncHook = asyncHooks.createHook({init, destroy})
const transactions = new Map()

asyncHook.enable()

function getCLS () {
  const asyncId = asyncHooks.executionAsyncId()
  return transactions.has(asyncId) ? transactions.get(asyncId) : null
}

function setCLS (value) {
  const asyncId = asyncHooks.executionAsyncId()
  transactions.set(asyncId, value)
}

function init (asyncId, type, triggerAsyncId, resource) {
  transactions.set(asyncId, getCLS())
}

function destroy (asyncId) {
  transactions.delete(asyncId)
}

http.createServer(function (req, res) {
  handler().then(function (data) {
    res.end(JSON.stringify(data))
  }).catch(function (err) {
    res.statusCode = 500
    res.end(err.stack)
  })
}).listen(3000)

var counter = 0

async function handler () {
  setCLS(counter++)
  await sleep(10)
  return { cls: getCLS() }
}

Correct behavior (node 8.11.1):

$ curl localhost:3000
{"cls":0}

master (with and without this patch):

$ curl localhost:3000
{"cls":null}

[BridgeAR]

@mcollina thanks for pointing that out. This is independent from this patch though, so the patch should land nevertheless as far as I see it.

[mcollina]

LGTM

[mcollina]

Reported as #20516.

[ofrobots]

This was merged upstream into V8 6.7 (Thanks @MylesBorins). Since we are still at V8 6.6 on master and on 10.x, I think this can land.

EDIT: so that it can get into a 10.x release next week.

[BridgeAR]

Comment addressed. PTAL @ofrobots

CI https://ci.nodejs.org/job/node-test-pull-request/14736/

[Conduitry]

Does this new test fail on master? Just want to check whether the test ought to assert that some of those ids are not equal to other ones.

edit: Oh, never mind probably. That's what the last few assertions are for, not sure how I missed that.

[mcollina]

@MayaLekova @bmeurer should this land? or should we wait for a more complete revert/fix?

[MayaLekova]

@mcollina Please don't merge this, it fixes one of the cases, but seems to generally worsen the validity of the code. And the complete revert is coming soon in V8.
Hope my reply doesn't come too late!

[BridgeAR]

@MayaLekova it is fine not to land this but it would be good to land the revert in 10.2.0 since this is a significant issue for some people.

[MylesBorins]

@BridgeAR what needs to be reverted?

[MayaLekova]

@BridgeAR which version of V8 is going to land in 10.2.0? Will have your comment in mind.

[MylesBorins]

We can land whatever the latest version of 6.6 is available

…

On Mon, May 14, 2018, 2:47 AM Maya Lekova ***@***.***> wrote: @BridgeAR <https://github.com/BridgeAR> which version of V8 is going to land in 10.2.0? Will have your comment in mind. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20467 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVzMzJpjteMHu9pfm6RB8Av3PmMOGks5tyShkgaJpZM4TvWEu> .

[ofrobots]

@MayaLekova Right now Node 10.x is using the V8 6.6 branch. Once V8 6.7 goes stable, we plan to bring that into the 10.x branch as well – but that is going to be no sooner than May 29-ish. For now, you can assume that the fixes are needed against V8 6.6.

If the fixes/reverts you're working on can be merged to V8 6.6 – that's great, we can just roll the V8 dep and pick those up. I suspect however – given that the revert is substantial – that upstream V8 would not want to merge back. In that case we can float the changes here in Node.js as a patch.

[MayaLekova]

Thank you for the info!
We discussed with Benedikt that floating the patch would be the better option for us. Will let you know by tomorrow evening if the change has landed successfully in V8.