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.

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

src: remove a stale comment in async_hooks #43317

Merged
merged 1 commit into from
Jun 30, 2022
Merged

src: remove a stale comment in async_hooks #43317

merged 1 commit into from
Jun 30, 2022

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Jun 5, 2022 •
edited
Loading

This removes a comment relevant to runtime checks for async_hooks.

Even if async_hooks is experimental, the check pointed by the comment
is performed as default unless --no-force-async-hooks-checks is given
from CLI arguments.

Refs: #16318
Refs: #15454 (comment)

Plus, the comment conflicts with the following L1071.

node/src/env.cc

Lines 1071 to 1076 in 45d7ca9

// Always perform async_hooks checks, not just when async_hooks is enabled.
// TODO(AndreasMadsen): Consider removing this for LTS releases.
// See discussion in https://github.com/nodejs/node/pull/15454
// When removing this, do it by reverting the commit. Otherwise the test
// and flag changes won't be included.
fields_[kCheck] = 1;

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

@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 Jun 5, 2022
@daeyeon daeyeon force-pushed the master.src.comment-220605.Sun.ec61 branch 3 times, most recently from 3d249ee to 4fd0cf8 Compare June 5, 2022 12:47
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jun 16, 2022
Open
22 tasks
@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 16, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Jun 17, 2022
Open
Open
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jun 19, 2022
Open
30 tasks
@daeyeon
src: remove a stale comment in async_hooks
c9a0477 
This removes a comment relevant to runtime checks for `async_hooks`.

Even if `async_hooks` is experimental, the check pointed by the comment
is performed as default unless `--no-force-async-hooks-checks` is given
from CLI arguments.

Refs: nodejs#16318
Refs: nodejs#15454 (comment)

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@daeyeon daeyeon force-pushed the master.src.comment-220605.Sun.ec61 branch from 4fd0cf8 to c9a0477 Compare June 19, 2022 12:19
@daeyeon
Copy link
Member Author

daeyeon commented Jun 19, 2022

I've rebased this PR's codebase since the previous one includes some flaky tests such as test_buffer/test_finalizer, test-net-connect-reset-until-connected, and so on. Seemingly, the flakinesses have been reduced with recent PRs. :-)

Could you re-run the CI?

@github-actions github-actions bot mentioned this pull request Jun 20, 2022
Open
36 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jun 21, 2022
Open
Open
Open
This was referenced Jun 24, 2022
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 merged commit 3b0995e into nodejs:main Jun 30, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jun 30, 2022

Landed in 3b0995e

@aduh95 aduh95 removed the needs-ci PRs that need a full CI run. label Jun 30, 2022
@github-actions github-actions bot mentioned this pull request Jul 4, 2022
Open
23 tasks
targos pushed a commit that referenced this pull request Jul 12, 2022
@daeyeon @targos
src: remove a stale comment in async_hooks
5ae30bf 
This removes a comment relevant to runtime checks for `async_hooks`.

Even if `async_hooks` is experimental, the check pointed by the comment
is performed as default unless `--no-force-async-hooks-checks` is given
from CLI arguments.

Refs: #16318
Refs: #15454 (comment)

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43317
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@daeyeon @targos
src: remove a stale comment in async_hooks
c5a822e 
This removes a comment relevant to runtime checks for `async_hooks`.

Even if `async_hooks` is experimental, the check pointed by the comment
is performed as default unless `--no-force-async-hooks-checks` is given
from CLI arguments.

Refs: #16318
Refs: #15454 (comment)

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43317
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@daeyeon @targos
src: remove a stale comment in async_hooks
26e4dc8 
This removes a comment relevant to runtime checks for `async_hooks`.

Even if `async_hooks` is experimental, the check pointed by the comment
is performed as default unless `--no-force-async-hooks-checks` is given
from CLI arguments.

Refs: #16318
Refs: #15454 (comment)

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43317
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@daeyeon @guangwong
src: remove a stale comment in async_hooks
0ee391a 
This removes a comment relevant to runtime checks for `async_hooks`.

Even if `async_hooks` is experimental, the check pointed by the comment
is performed as default unless `--no-force-async-hooks-checks` is given
from CLI arguments.

Refs: nodejs/node#16318
Refs: nodejs/node#15454 (comment)

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: nodejs/node#43317
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanarbol juanarbol juanarbol approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@daeyeon @nodejs-github-bot @aduh95 @jasnell @lpinca @juanarbol @RaisinTen