Skip to content

fs: replace SetMethodNoSideEffect in node_file.cc#49857

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 1 commit into from
Sep 27, 2023
Merged

fs: replace SetMethodNoSideEffect in node_file.cc#49857

nodejs-github-bot merged 1 commit into from
Sep 27, 2023

Conversation

@CanadaHonk
Copy link
Member

@CanadaHonkCanadaHonk commented Sep 25, 2023

All FS methods should not use it since FS operations will have side effects (suggested by @anonrig)

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Sep 25, 2023
@anonrig
Copy link
Member

anonrig commented Sep 25, 2023

cc @joyeecheung

@anonriganonrig added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 25, 2023
@github-actions

This comment was marked as outdated.

Sign in to view
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 25, 2023

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

@CanadaHonkCanadaHonkforce-pushed the fs-replace-nosideeffect branch from 2cf762e to bf379efCompareSeptember 25, 2023 13:19
@benjamingr
Copy link
Member

benjamingr commented Sep 25, 2023

Why is this being fast tracked does it unblock something?

@CanadaHonk
Copy link
MemberAuthor

CanadaHonk commented Sep 25, 2023

Why is this being fast tracked does it unblock something?

Pretty sure it's because it fixes repl possibly doing unwanted real FS ops for a few functions if you were to type out but not press enter (eg fs.copyFileSync(...)).

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 25, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 25, 2023

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

@cjihrig
Copy link
Contributor

cjihrig commented Sep 25, 2023

The fact that this fixes a bug does not really mean that it qualifies for fast tracking.

@cjihrigcjihrig removed the fast-track PRs that do not need to wait for 48 hours to land. label Sep 25, 2023
@targos
Copy link
Member

targos commented Sep 25, 2023•
edited
Loading

  • The side-effect of copyFileSync is obvious.
  • I guess that readFileUtf8 can modify the atime of the file
  • But what are the observable side-effects of accessSync and existsSync ?

@anonrig
Copy link
Member

anonrig commented Sep 25, 2023

But what are the observable side-effects of accessSync and existsSync ?

There isn't but referring to @joyeecheung's message (ref: #49748 (comment)), I think it is unnecessary to run fs operations (since they're costly) on REPL.

@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 25, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Sep 26, 2023

but referring to @joyeecheung's message

I don't interpret @joyeecheung's comment (but please correct me if I'm wrong) to mean we shouldn't do it because of performance reasons. I interpret it as chown() having very real side effects.

I think it is unnecessary to run fs operations (since they're costly) on REPL.

I don't feel strongly about this, but I think this is another case where performance just isn't important. The REPL is meant to be interactive and a person typing fs.accessSync() or fs.existsSync() is very unlikely to notice any difference in performance.

@github-actionsgithub-actionsbot mentioned this pull request Sep 27, 2023
Open
21 tasks
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 27, 2023
@nodejs-github-botnodejs-github-bot merged commit 783f64b into nodejs:mainSep 27, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 27, 2023

Landed in 783f64b

@CanadaHonkCanadaHonk mentioned this pull request Sep 27, 2023
Closed
@github-actionsgithub-actionsbot mentioned this pull request Sep 28, 2023
Open
19 tasks
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@CanadaHonk@ruyadorno
fs: replace SetMethodNoSideEffect in node_file
9363179
PR-URL: #49857 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@ruyadornoruyadorno mentioned this pull request Sep 28, 2023
Closed
ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
@CanadaHonk@ruyadorno
fs: replace SetMethodNoSideEffect in node_file
ec323bb
PR-URL: #49857 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@ruyadornoruyadorno mentioned this pull request Sep 28, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@CanadaHonk@alexfernandez
fs: replace SetMethodNoSideEffect in node_file
c7f1635
PR-URL: nodejs#49857 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
@CanadaHonk@debadree25
fs: replace SetMethodNoSideEffect in node_file
554bc6e
PR-URL: nodejs#49857 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonriganonriganonrig approved these changes

@targostargostargos 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.c++Issues and PRs that require attention from people who are familiar with C++.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@CanadaHonk@anonrig@nodejs-github-bot@benjamingr@cjihrig@targos