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

deps: V8: cherry-pick eec10a2fd8fa #33778

Closed
wants to merge 1 commit into from

Conversation

Qard
Copy link
Member

@Qard Qard commented Jun 6, 2020

Original commit message:

[promisehook] Add before/after hooks to thenable tasks

This will allow Node.js to properly track async context in thenables.

Change-Id: If441423789a78307a57ad7e645daabf551cddb57
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
Commit-Queue: Gus Caplan <me@gus.host>
Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2

Original commit message:

    [promisehook] Add before/after hooks to thenable tasks

    This will allow Node.js to properly track async context in thenables.

    Change-Id: If441423789a78307a57ad7e645daabf551cddb57
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
    Commit-Queue: Gus Caplan <me@gus.host>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2
@Qard Qard added the v8 engine Issues and PRs related to the V8 dependency. label Jun 6, 2020
@Qard Qard requested review from targos and devsnek June 6, 2020 21:16
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 6, 2020
@nodejs-github-bot
Copy link
Collaborator

@Qard Qard added the async_hooks Issues and PRs related to the async hooks subsystem. label Jun 6, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Flarna
Copy link
Member

Flarna commented Jun 8, 2020

@Qard I'm curious why you started CI again? https://ci.nodejs.org/job/node-test-pull-request/31756/ was already green 😕

@Qard
Copy link
Member Author

Qard commented Jun 8, 2020

Just misread the status things on Github. There was a failure, but it wasn't from the Jenkins CI, it was from the GitHub Actions CI.

@Qard
Copy link
Member Author

Qard commented Jun 8, 2020

ARM CI doesn't seem to want to behave now. 😅

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Qard
Copy link
Member Author

Qard commented Jun 9, 2020

What's the process for cherry-picking V8 commits to other release lines? I'd like to backport to LTS too. Do I follow the regular backport process from this, or do I do a separate cherry-pick PR like this one just targetting the other branch?

@Flarna
Copy link
Member

Flarna commented Jun 9, 2020

triggered CI a few times and now it is at least not red anymore. Not sure if this is enough to land this.

@Qard
Copy link
Member Author

Qard commented Jun 11, 2020

I see a strange ${STATUS_LABEL} failure. Not sure what that's about. 😕

@Flarna
Copy link
Member

Flarna commented Jun 12, 2020

As far as I see the last CI run was not red so this should be ready to go. I tool a look in a few other PRs recently merged and all of them had a yellow CI.

@targos
Copy link
Member

targos commented Jun 13, 2020

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 13, 2020
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Flarna
Copy link
Member

Flarna commented Aug 4, 2020

In general I also don't like breaking major modules.
But if I understood this correct it's already broken since 12.18.2 (see #31745 (comment)).

@Qard
Copy link
Member Author

Qard commented Aug 4, 2020

Ah, missed that it traced back to that. If it's not caused by this then I see no reason not to backport.

@Flarna
Copy link
Member

Flarna commented Aug 14, 2020

I was able to get access to some MacOs machine and executed gulp and glob-watcher tests.
For me they fail even with node.js 10.22.0, 14.4.0, 12.18.1 so it seems they are broken since quite a while and no relation to this PR.

I will create a backport PR for this in a while.

@Flarna
Copy link
Member

Flarna commented Aug 14, 2020

Seems my testing done above was not done correct. Once tests of glob-watcher fail it's needed to cleanup via git clean -fdx test/ otherwise all followup runs fail.

With this done right it looks different:

  • 10.22.0 OK
  • 12.18.1 OK
  • 12.18.2 NOK assert in uv_close sometimes
  • 14.4.0 sporadic segmentation faults
  • 14.5.0 assert in uv_close sometimes (in addition to segfaults from 14.4.0)

Flarna pushed a commit to dynatrace-oss-contrib/node that referenced this pull request Sep 23, 2020
Original commit message:

    [promisehook] Add before/after hooks to thenable tasks

    This will allow Node.js to properly track async context in thenables.

    Change-Id: If441423789a78307a57ad7e645daabf551cddb57
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
    Commit-Queue: Gus Caplan <me@gus.host>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2

PR-URL: nodejs#33778
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Flarna added a commit to dynatrace-oss-contrib/node that referenced this pull request Sep 23, 2020
This adds a test to verify that AsyncLocalStorage works with thenables.

PR-URL: nodejs#34008
Refs: nodejs#33778
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 23, 2020
Original commit message:

    [promisehook] Add before/after hooks to thenable tasks

    This will allow Node.js to properly track async context in thenables.

    Change-Id: If441423789a78307a57ad7e645daabf551cddb57
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
    Commit-Queue: Gus Caplan <me@gus.host>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2
Backport-PR-URL: #34776
PR-URL: #33778
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit that referenced this pull request Sep 23, 2020
This adds a test to verify that AsyncLocalStorage works with thenables.

Backport-PR-URL: #34776
PR-URL: #34008
Refs: #33778
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Flarna pushed a commit to dynatrace-oss-contrib/node that referenced this pull request Sep 28, 2020
Original commit message:

    [promisehook] Add before/after hooks to thenable tasks

    This will allow Node.js to properly track async context in thenables.

    Change-Id: If441423789a78307a57ad7e645daabf551cddb57
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
    Commit-Queue: Gus Caplan <me@gus.host>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2

PR-URL: nodejs#33778
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
richardlau pushed a commit that referenced this pull request Oct 7, 2020
Original commit message:

    [promisehook] Add before/after hooks to thenable tasks

    This will allow Node.js to properly track async context in thenables.

    Change-Id: If441423789a78307a57ad7e645daabf551cddb57
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2215624
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
    Commit-Queue: Gus Caplan <me@gus.host>
    Cr-Commit-Position: refs/heads/master@{#68207}

Refs: v8/v8@eec10a2

PR-URL: #33778
Backport-PR-URL: #35393
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@richardlau richardlau mentioned this pull request Oct 7, 2020
3 tasks
@astorm
Copy link

astorm commented Jan 5, 2021

@Qard Thanks for this -- we've (Elastic APM) had a few users/customers notice the improvement in their Knex instrumentation due to this patch. </drivebypraise>

@kibertoad
Copy link
Contributor

@astorm Glad to hear that! If there is something on a Knex side that would be needed to make the instrumentation work better, we (Knex dev team) would be more than happy to do our part as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.