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

cluster: use ObjectPrototypeHasOwnProperty #48141

Merged
merged 2 commits into from
May 25, 2023
Merged

cluster: use ObjectPrototypeHasOwnProperty #48141

merged 2 commits into from
May 25, 2023

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented May 23, 2023

This addresses: #48077 (comment)

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

@daeyeon
cluster: use ObjectPrototypeHasOwnProperty
83c4b73 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. needs-ci PRs that need a full CI run. labels May 23, 2023
@Trott Trott added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 23, 2023
@Trott
Copy link
Member

Trott commented May 23, 2023

Should we add a test that modifies the prototype chain? (I generally prefer to add tests, but I can also see an argument that it might be a bit much to always add tests for primordials. I guess I'd prefer a test be added, but not so strongly that I'd block this landing without a test.)

@anonrig
Copy link
Member

anonrig commented May 24, 2023

I'm +1 on adding tests as well.

@daeyeon
Copy link
Member Author

daeyeon commented May 24, 2023 •
edited
Loading

On second thought as I added the test, I think that the builtin doesn't prevent changes to the process.env prototype. Closing as we don't have a clue it needs to be applied to cluster only. Thanks for the review!

@daeyeon daeyeon closed this May 24, 2023
@daeyeon daeyeon deleted the main.has-own-230523.Tue.862f branch May 24, 2023 00:53
@Trott
Copy link
Member

Trott commented May 24, 2023

On second thought as I added the test, I think that the builtin doesn't prevent changes to the process.env prototype. Closing as we don't have a clue it needs to be applied to cluster only. Thanks for the review!

We can't avoid changes to process.env prototype, but we can avoid those prototype changes affecting the behavior of the cluster module and other modules. That's what this is about, right? I think we should re-open this.

@daeyeon
fixup! cluster: use ObjectPrototypeHasOwnProperty
5f04200 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon restored the main.has-own-230523.Tue.862f branch May 25, 2023 09:11
@daeyeon daeyeon reopened this May 25, 2023
@daeyeon
Copy link
Member Author

daeyeon commented May 25, 2023

Yes, that's right. This way is safer, but I thought there might be a reason why the properties of process.env are just used in the other builtin modules. Maybe I was overly concerned.. Re-opened this with a test. PTAL.

@daeyeon daeyeon added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels May 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented May 25, 2023

This way is safer, but I thought there might be a reason why the properties of process.env are just used in the other builtin modules.

It's possible that there would be significant negative performance implications in some hot paths, but I don't think this is one of those.

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels May 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2023
@nodejs-github-bot nodejs-github-bot merged commit b4d5f1f into nodejs:main May 25, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in b4d5f1f

@daeyeon daeyeon deleted the main.has-own-230523.Tue.862f branch May 26, 2023 04:24
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request May 26, 2023
@daeyeon @marco-ippolito
cluster: use ObjectPrototypeHasOwnProperty
b793148 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: nodejs#48141
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos pushed a commit that referenced this pull request May 30, 2023
@daeyeon @targos
cluster: use ObjectPrototypeHasOwnProperty
6cf2adc 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #48141
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos targos mentioned this pull request Jun 4, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@daeyeon @danielleadams
cluster: use ObjectPrototypeHasOwnProperty
1b248f3 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: #48141
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@daeyeon @MoLow
cluster: use ObjectPrototypeHasOwnProperty
7692d2e 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: nodejs#48141
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@daeyeon @Ceres6
cluster: use ObjectPrototypeHasOwnProperty
da080a8 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: nodejs#48141
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@daeyeon @Ceres6
cluster: use ObjectPrototypeHasOwnProperty
167fb56 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
PR-URL: nodejs#48141
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@Trott Trott Trott 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. cluster Issues and PRs related to the cluster subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daeyeon @Trott @anonrig @nodejs-github-bot