Skip to content

async_hooks: remove internal only error checking#30967

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

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

Conversation

@apapirovski
Copy link
Copy Markdown
Contributor

@apapirovski apapirovski commented Dec 14, 2019 •
edited
Loading

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-bot nodejs-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
Copy link
Copy Markdown
Member

Trott commented Dec 14, 2019 •
edited
Loading

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

@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski commented Dec 15, 2019

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
Copy link
Copy Markdown
Member

Trott commented Dec 21, 2019

@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
Copy link
Copy Markdown
Member

@Trott Trott left a comment

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.

@BridgeAR BridgeAR 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
Copy link
Copy Markdown
Member

BridgeAR commented Jan 1, 2020

Ping @apapirovski

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski commented Jan 8, 2020

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
Copy link
Copy Markdown
Collaborator

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

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

@apapirovski apapirovski 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
Copy link
Copy Markdown
Contributor Author

apapirovski commented Jan 11, 2020

The latest CI posted above passed without any issues.

@Trott
Copy link
Copy Markdown
Member

Trott commented Jan 12, 2020

Landed in 4de31d5

@Trott Trott 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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere 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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request Jun 1, 2021
Open
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

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

No one assigned

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.

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