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

async_hooks: eliminate require side effects #40782

Merged

Conversation

Qard
Copy link
Member

@Qard Qard commented Nov 10, 2021

Just a minor improvement to avoid any unintended side effects to requiring async_hooks.

cc @nodejs/async_hooks

@Qard Qard added the async_hooks Issues and PRs related to the async hooks subsystem. label Nov 10, 2021
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Nov 10, 2021
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@targos
Copy link
Member

targos commented Nov 11, 2021

Is this related to https://twitter.com/evanhlucas/status/1458192210382528518 ?

@Qard
Copy link
Member Author

Qard commented Nov 11, 2021

I had a quick look because of that. As far as I can tell this shouldn't trigger that though. I just saw a thing in looking back at the code that could be improved.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2021
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Flarna Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 11, 2021
@Trott
Copy link
Member

Trott commented Nov 12, 2021

Does it make sense to add a test?

@Qard
Copy link
Member Author

Qard commented Nov 12, 2021

Not sure how you'd even test it given that it's functionally identical other than just setting the trampoline function a little later and clearing it if the hooks are disabled. Do we have prior art on testing these environment fields? I guess it'd need to be a native test.

In any case, I'll be travelling for the next week so I can either deal with that after I get back or we can land as-is. I'm good either way as it's not really any substantial/important change.

PR-URL: nodejs#40782
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@Trott Trott force-pushed the eliminate-async_hooks-require-side-effects branch from 15a34d4 to 689405c Compare November 13, 2021 14:37
@Trott
Copy link
Member

Trott commented Nov 13, 2021

Landed in 689405c

@Trott Trott merged commit 689405c into nodejs:master Nov 13, 2021
@Qard Qard deleted the eliminate-async_hooks-require-side-effects branch November 17, 2021 13:08
targos pushed a commit that referenced this pull request Nov 21, 2021
PR-URL: #40782
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
PR-URL: #40782
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #40782
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
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. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants