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: keep fs.promises.readFile read until EOF is reached #52178

Merged
merged 1 commit into from
May 11, 2024
Merged

fs: keep fs.promises.readFile read until EOF is reached #52178

merged 1 commit into from
May 11, 2024

Conversation

kylo5aby
Copy link
Contributor

  • For cases where the size==0 of the file to read, ensure it ends upon reaching EOF.

Fixes: #52155

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 21, 2024
@kylo5aby kylo5aby force-pushed the fs-readfile branch 2 times, most recently from 7e6ce28 to 212c845 Compare March 22, 2024 08:14
@H4ad
Copy link
Member

H4ad commented Mar 22, 2024

The PR looks great, can you add a test just to ensure the issue is fixed?

BridgeAR
BridgeAR reviewed Mar 22, 2024
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. I guess we can improve the conditions a tad with my suggestions and I wonder if we have to do the same for the non-encoding situation. Is that also impacted? Let's add regression tests for both to be safe.

lib/internal/fs/promises.js
@@ -557,7 +558,7 @@ async function readFileHandle(filehandle, options) {

if (bytesRead === 0 ||
totalRead === size ||
(bytesRead !== buffer.length && !chunkedRead)) {
(bytesRead !== buffer.length && !chunkedRead && !noSize)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about changing the chunkedRead above instead:

const chunkedRead = length > kReadFileBufferLength || size === 0;

That would automatically check for it and it's one boolean check in the loop less.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the premise here for chunkedRead is already knowing the size of the file, meaning size === 0 cannot imply the need for chunkedRead, have I misunderstand something?

lib/internal/fs/promises.js Outdated
Comment on lines 586 to 587
result += decoder.write(noSize && bytesRead !== kReadFileUnknownBufferLength ?
buffer.subarray(0, bytesRead) : buffer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result += decoder.write(noSize && bytesRead !== kReadFileUnknownBufferLength ?
buffer.subarray(0, bytesRead) : buffer);
const writeBuffer = bytesRead !== buffer.length ?
buffer.subarray(0, bytesRead) :
buffer;
result += decoder.write(writeBuffer);

@kylo5aby
Copy link
Contributor Author

This is looking good. I guess we can improve the conditions a tad with my suggestions and I wonder if we have to do the same for the non-encoding situation. Is that also impacted? Let's add regression tests for both to be safe.

@kylo5aby kylo5aby closed this Mar 25, 2024
@kylo5aby kylo5aby reopened this Mar 25, 2024
@kylo5aby
Copy link
Contributor Author

This is looking good. I guess we can improve the conditions a tad with my suggestions and I wonder if we have to do the same for the non-encoding situation. Is that also impacted? Let's add regression tests for both to be safe.

Thanks for the reminder :)

@kylo5aby
Copy link
Contributor Author

I have added some regression tests. PTAL @H4ad @BridgeAR

@kylo5aby
Copy link
Contributor Author

could you take a look? @legendecas

legendecas
legendecas reviewed Apr 16, 2024
View reviewed changes
lib/internal/fs/promises.js Outdated
@@ -580,15 +581,17 @@ async function readFileHandle(filehandle, options) {
result += decoder.end(buffer);
return result;
}

const writeBuffer = bytesRead !== buffer.length ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this is not a buffer for a "write" operation. Instead, it is a result of "read".

Suggested change
const writeBuffer = bytesRead !== buffer.length ?
const readBuffer = bytesRead !== buffer.length ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved. Thanks!

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2024
@nodejs-github-bot nodejs-github-bot merged commit ff7910b into nodejs:main May 11, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ff7910b

targos pushed a commit that referenced this pull request May 11, 2024
@kylo5aby @targos
fs: keep fs.promises.readFile read until EOF is reached
f076e72 
PR-URL: #52178
Fixes: #52155
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targos targos mentioned this pull request May 13, 2024
Merged
@orgads
Copy link
Contributor

orgads commented May 27, 2024

Can this be backported to v20?

lukins-cz pushed a commit to lukins-cz/OS-Aplet-node that referenced this pull request Jun 1, 2024
@kylo5aby @lukins-cz
fs: keep fs.promises.readFile read until EOF is reached
58d69e2 
PR-URL: nodejs#52178
Fixes: nodejs#52155
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@legendecas legendecas legendecas approved these changes

Assignees
No one assigned
Labels
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.

[REG 18.10.0->18.11.0] fs.promises.readFile from stdin is broken for slow input
7 participants
@kylo5aby @H4ad @nodejs-github-bot @orgads @legendecas @BridgeAR @aduh95