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

fs: replace SetMethodNoSideEffect in node_file.cc #49857

Merged
merged 1 commit into from Sep 27, 2023
Merged

fs: replace SetMethodNoSideEffect in node_file.cc #49857

merged 1 commit into from Sep 27, 2023

Conversation

CanadaHonk
Copy link
Member

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

@nodejs-github-bot nodejs-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

@anonrig anonrig 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-actions github-actions bot 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

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

@benjamingr
Copy link
Member

Why is this being fast tracked does it unblock something?

@CanadaHonk
Copy link
Member Author

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(...)).

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@github-actions github-actions bot 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

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

@nodejs-github-bot
Copy link
Collaborator

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.

@cjihrig cjihrig 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

  • 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.

@anonrig anonrig 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-actions github-actions bot mentioned this pull request Sep 27, 2023
Open
21 tasks
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 27, 2023
@nodejs-github-bot nodejs-github-bot merged commit 783f64b into nodejs:main Sep 27, 2023
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 783f64b

@CanadaHonk CanadaHonk mentioned this pull request Sep 27, 2023
Open
@github-actions github-actions bot 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 <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@ruyadorno ruyadorno 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 <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@ruyadorno ruyadorno 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 <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.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

@targos targos targos 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. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@CanadaHonk @anonrig @nodejs-github-bot @benjamingr @cjihrig @targos