Skip to content

async_hooks: executionAsyncResource matches in hooks #31821

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

async_hooks: executionAsyncResource matches in hooks #31821

wants to merge 1 commit into from

Conversation

@Flarna
Copy link
Member

@Flarna Flarna commented Feb 16, 2020

Ensure that resource returned by executionAsyncResource() in before and after hook matches that resource causing this before/after calls.

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

Refs: #30959
fyi @Qard

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 16, 2020
@Flarna Flarna requested a review from Qard February 16, 2020 23:22
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 17, 2020

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

vdeturckheim
vdeturckheim approved these changes Feb 18, 2020
View reviewed changes
Copy link
Member

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot @Flarna !

@vdeturckheim vdeturckheim added the async_hooks Issues and PRs related to the async hooks subsystem. label Feb 23, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 23, 2020

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

@Flarna
Copy link
Member Author

Flarna commented Feb 23, 2020

Seems the test I added here fails in node-test-commit-custom-suites-freestyle. Will take a look later. Any hints whats spacial about this CI job are welcome.

@addaleax
Copy link
Member

addaleax commented Feb 23, 2020

@Flarna Not sure why the job is named that way but it’s basically running each test inside a Worker thread, i.e. python tools/test.py --worker path/to/test

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2020

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

@vdeturckheim
Copy link
Member

vdeturckheim commented Feb 26, 2020

@Flarna the issue seems fixed, is it ready for a rebase?

@Flarna
Copy link
Member Author

Flarna commented Feb 26, 2020

From my point of view it's ready for landing.

@vdeturckheim
Copy link
Member

vdeturckheim commented Feb 26, 2020

@Flarna can you please squash it to a single commit?

@Flarna
Copy link
Member Author

Flarna commented Feb 26, 2020

Sure, but why is this needed here?
Till now this happened during landing. At least I was never asked before to squash on private branch in node repo.

@vdeturckheim
Copy link
Member

vdeturckheim commented Feb 26, 2020

It's indeed doable at merge time but I don't like rewriting other people's commits 😅

@Flarna
async_hooks: executionAsyncResource matches in hooks
19fbe32 
Ensure that resource returned by executionAsyncResource() in before
and after hook matches that resource causing this before/after calls.
@Flarna Flarna force-pushed the async-exe-resoure-match branch from cbe78d1 to 19fbe32 Compare February 26, 2020 12:13
@Flarna
Copy link
Member Author

Flarna commented Feb 26, 2020

Ok, rebased and squashed. Not sure if CI needs to run again now.

I was assuming that squashin is done by some landing script or similar. Commits are rewritten anyway during landing as PR url, list of revierers is added.

@vdeturckheim
Copy link
Member

vdeturckheim commented Feb 26, 2020

Thanks @Flarna , I'll restart CI.
Yep commits are updated, but it is automatically done by a script.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 26, 2020

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

@vdeturckheim vdeturckheim added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 26, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 26, 2020

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

@vdeturckheim
Copy link
Member

vdeturckheim commented Feb 26, 2020

Landed in fd3d02a

vdeturckheim pushed a commit that referenced this pull request Feb 26, 2020
@Flarna @vdeturckheim
async_hooks: executionAsyncResource matches in hooks
fd3d02a 
Ensure that resource returned by executionAsyncResource() in before
and after hook matches that resource causing this before/after calls.

PR-URL: #31821
Refs: #30959
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
@Flarna Flarna deleted the async-exe-resoure-match branch February 26, 2020 14:17
codebytere pushed a commit that referenced this pull request Feb 27, 2020
@Flarna @codebytere
async_hooks: executionAsyncResource matches in hooks
6af9e7e 
Ensure that resource returned by executionAsyncResource() in before
and after hook matches that resource causing this before/after calls.

PR-URL: #31821
Refs: #30959
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
@codebytere codebytere mentioned this pull request Feb 29, 2020
Merged
@targos targos added backported-to-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 7, 2020
@puzpuzpuz puzpuzpuz mentioned this pull request Mar 7, 2020
Closed
4 tasks
puzpuzpuz pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 2020
@Flarna @puzpuzpuz
async_hooks: executionAsyncResource matches in hooks
0a7da1c 
Ensure that resource returned by executionAsyncResource() in before
and after hook matches that resource causing this before/after calls.

PR-URL: nodejs#31821
Refs: nodejs#30959
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
puzpuzpuz pushed a commit to puzpuzpuz/node that referenced this pull request Apr 14, 2020
@Flarna @puzpuzpuz
async_hooks: executionAsyncResource matches in hooks
1f61d3c 
Ensure that resource returned by executionAsyncResource() in before
and after hook matches that resource causing this before/after calls.

PR-URL: nodejs#31821
Refs: nodejs#30959
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@Flarna @targos
async_hooks: executionAsyncResource matches in hooks
89dc234 
Ensure that resource returned by executionAsyncResource() in before
and after hook matches that resource causing this before/after calls.

PR-URL: nodejs#31821
Refs: nodejs#30959
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@Flarna @targos
async_hooks: executionAsyncResource matches in hooks
b4ca132 
Ensure that resource returned by executionAsyncResource() in before
and after hook matches that resource causing this before/after calls.

PR-URL: #31821
Refs: #30959
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Qard Qard Qard approved these changes

@addaleax addaleax addaleax approved these changes

@JungMinu JungMinu JungMinu approved these changes

@legendecas legendecas legendecas approved these changes

+1 more reviewer

@vdeturckheim vdeturckheim vdeturckheim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@Flarna @nodejs-github-bot @addaleax @vdeturckheim @Qard @JungMinu @legendecas @targos