Skip to content

cluster: use ObjectPrototypeHasOwnProperty#48141

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
Merged
nodejs-github-bot merged 2 commits into from
May 25, 2023
Merged

cluster: use ObjectPrototypeHasOwnProperty #48141

nodejs-github-bot merged 2 commits into from
May 25, 2023

Conversation

@daeyeon
Copy link
Member

@daeyeondaeyeon commented May 23, 2023

This addresses: #48077 (comment)

Signed-off-by: Daeyeon Jeong [email protected]

@nodejs-github-botnodejs-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
@TrottTrott 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
MemberAuthor

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!

@daeyeondaeyeon closed this May 24, 2023
@daeyeondaeyeon 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.

@daeyeondaeyeon restored the main.has-own-230523.Tue.862f branch May 25, 2023 09:11
@daeyeondaeyeon reopened this May 25, 2023
@daeyeon
Copy link
MemberAuthor

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.

@daeyeondaeyeon 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-actionsgithub-actionsbot 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

nodejs-github-bot commented May 25, 2023

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.

@anonriganonrig 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-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 25, 2023
@nodejs-github-botnodejs-github-bot merged commit b4d5f1f into nodejs:mainMay 25, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 25, 2023

Landed in b4d5f1f

@daeyeondaeyeon 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 <[email protected]> PR-URL: nodejs#48141 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this pull request May 30, 2023
@daeyeon@targos
cluster: use ObjectPrototypeHasOwnProperty
6cf2adc
Signed-off-by: Daeyeon Jeong <[email protected]> PR-URL: #48141 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
@targostargos 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 <[email protected]> PR-URL: #48141 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
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 <[email protected]> PR-URL: nodejs#48141 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
@danielleadamsdanielleadams 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 <[email protected]> PR-URL: nodejs#48141 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
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 <[email protected]> PR-URL: nodejs#48141 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes

@anonriganonriganonrig approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.clusterIssues and PRs related to the cluster subsystem.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs 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