FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: keep fs.promises.readFile read until EOF is reached by kylo5aby · Pull Request #52178 · nodejs/node · GitHub

/ node Public

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

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:fs-readfile
May 11, 2024
ConversationCommits (1)ChecksFiles changed
Merged

fs: keep fs.promises.readFile read until EOF is reached#52178
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:fs-readfile

Conversation

kylo5aby commented Mar 21, 2024

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

Fixes: #52155

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 force-pushed the fs-readfile branch 2 times, most recently from 7e6ce28 to 212c845 Compare March 22, 2024 08:14

H4ad commented Mar 22, 2024

Copy link
Copy Markdown
Member

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

BridgeAR reviewed Mar 22, 2024
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/internal/fs/promises.js Outdated

BridgeAR Mar 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

kylo5aby Mar 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread lib/internal/fs/promises.js Outdated
Comment on lines 586 to 587

BridgeAR Mar 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
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 commented Mar 25, 2024

Copy link
Copy Markdown
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 closed this Mar 25, 2024
kylo5aby reopened this Mar 25, 2024

kylo5aby commented Mar 25, 2024

Copy link
Copy Markdown
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 commented Mar 25, 2024

Copy link
Copy Markdown
Contributor Author

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

kylo5aby commented Apr 16, 2024

Copy link
Copy Markdown
Contributor Author

could you take a look? @legendecas

legendecas reviewed Apr 16, 2024
View reviewed changes
Comment thread lib/internal/fs/promises.js Outdated

legendecas Apr 16, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 ?

kylo5aby Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Resolved. Thanks!

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2024
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 commented Apr 17, 2024

Copy link
Copy Markdown
Collaborator

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

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2024
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 merged commit ff7910b into nodejs:main May 11, 2024

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

Landed in ff7910b

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

orgads commented May 27, 2024

Copy link
Copy Markdown
Contributor

Can this be backported to v20?

marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
fs: keep fs.promises.readFile read until EOF is reached
7d38c2e 
PR-URL: #52178
Fixes: #52155
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
fs: keep fs.promises.readFile read until EOF is reached
7e580b2 
PR-URL: nodejs#52178
Fixes: nodejs#52155
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
coderabbitai Bot mentioned this pull request May 19, 2026
Merged
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR left review comments
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

Back | FazBrowse Home | New Git URL