Skip to content

async_hooks: remove internal only error checking - #30967

Closed
apapirovski wants to merge 3 commits into
nodejs:masterfrom
apapirovski:remove-internal-ah-type-validation
Closed

async_hooks: remove internal only error checking#30967
apapirovski wants to merge 3 commits into
nodejs:masterfrom
apapirovski:remove-internal-ah-type-validation

Conversation

@apapirovski

@apapirovskiapapirovski commented Dec 14, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

This error checking is mostly unnecessary and is just a Node core developer nicety, rather than something that is needed for the user-land. It can be safely removed without any practical impact while making nextTick, timers, immediates and AsyncResource substantially faster.

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

@apapirovski
async_hooks: remove internal only error checking
5dbd853
This error checking is mostly unnecessary and is just a Node core
developer nicety, rather than something that is needed for the
user-land. It can be safely removed without any practical
impact while making nextTick, timers, immediates and AsyncResource
substantially faster.
@nodejs-github-botnodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Dec 14, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott

Trott commented Dec 14, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Not sure but CI results suggest that running tools/test.py --worker might produce a relevant failure?

@apapirovski

apapirovski commented Dec 15, 2019

Copy link
Copy Markdown
ContributorAuthor

Not sure but CI results suggest that running tools/testpy --worker might produce a relevant failure?

Kinda. I just overestimated the ability to remove the current common.skip so I guess back to having that in there.

@Trott

Trott commented Dec 21, 2019

Copy link
Copy Markdown
Member

@apapirovski Can you add back the one bit of test-skipping code for workers so we can re-run CI and get this landed?

Trott
Trott approved these changes Dec 21, 2019
View reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM once we get the test skipped in workers again.

@BridgeARBridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. wip Issues and PRs that are still a work in progress. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 24, 2019
@BridgeAR

BridgeAR commented Jan 1, 2020

Copy link
Copy Markdown
Member

Ping @apapirovski

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@apapirovski

apapirovski commented Jan 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Sorry for the delay @Trott@BridgeAR — didn't want to do the lazy fix and didn't have time with my move to get around to this until now. This test can now run with workers too.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Jan 8, 2020•
edited by BridgeAR
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28303/ ✅ (yellow build with a known flake)

@apapirovskiapapirovski added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Jan 11, 2020
@apapirovski

apapirovski commented Jan 11, 2020

Copy link
Copy Markdown
ContributorAuthor

The latest CI posted above passed without any issues.

@Trott

Trott commented Jan 12, 2020

Copy link
Copy Markdown
Member

Landed in 4de31d5

@TrottTrott closed this Jan 12, 2020
Trott pushed a commit that referenced this pull request Jan 12, 2020
@apapirovski@Trott
async_hooks: remove internal only error checking
4de31d5
This error checking is mostly unnecessary and is just a Node core
developer nicety, rather than something that is needed for the
user-land. It can be safely removed without any practical
impact while making nextTick, timers, immediates and AsyncResource
substantially faster.
PR-URL: #30967
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
@apapirovski@MylesBorins
async_hooks: remove internal only error checking
9d26358
This error checking is mostly unnecessary and is just a Node core
developer nicety, rather than something that is needed for the
user-land. It can be safely removed without any practical
impact while making nextTick, timers, immediates and AsyncResource
substantially faster.
PR-URL: #30967
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebyterecodebytere mentioned this pull request Jan 16, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 14, 2020
@apapirovski@codebytere
async_hooks: remove internal only error checking
0b3bee5
This error checking is mostly unnecessary and is just a Node core
developer nicety, rather than something that is needed for the
user-land. It can be safely removed without any practical
impact while making nextTick, timers, immediates and AsyncResource
substantially faster.
PR-URL: #30967
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@apapirovski@codebytere
async_hooks: remove internal only error checking
597431b
This error checking is mostly unnecessary and is just a Node core
developer nicety, rather than something that is needed for the
user-land. It can be safely removed without any practical
impact while making nextTick, timers, immediates and AsyncResource
substantially faster.
PR-URL: #30967
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebyterecodebytere mentioned this pull request Mar 17, 2020
Merged
@snyk-botsnyk-bot mentioned this pull request Jun 1, 2021
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QardQardQard approved these changes
@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

Labels

async_hooksIssues and PRs related to the async hooks subsystem.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@apapirovski@nodejs-github-bot@Trott@BridgeAR@Qard@jasnell