GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fs: keep fs.promises.readFile read until EOF is reached by kylo5aby · Pull Request #52178 · nodejs/node

Conversation

@kylo5aby

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and ACN-zhongqiang-li reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. 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

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BridgeAR
BridgeAR reviewed Mar 22, 2024
View reviewed changes

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/fs/promises.js Outdated

@BridgeAR BridgeAR Mar 22, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kylo5aby kylo5aby Mar 25, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/fs/promises.js Outdated
Comment on lines 586 to 587

@BridgeAR BridgeAR Mar 22, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kylo5aby

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kylo5aby kylo5aby closed this Mar 25, 2024
@kylo5aby kylo5aby reopened this Mar 25, 2024
@kylo5aby
kylo5aby force-pushed the fs-readfile branch from 212c845 to 5d35b0b Compare March 25, 2024 07:05
@kylo5aby

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kylo5aby

kylo5aby commented Mar 25, 2024

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kylo5aby
kylo5aby force-pushed the fs-readfile branch from 5d35b0b to c9cbbd1 Compare March 27, 2024 02:46
@kylo5aby

kylo5aby commented Apr 16, 2024

Copy link
Copy Markdown
Contributor Author

could you take a look? @legendecas
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas
legendecas reviewed Apr 16, 2024
View reviewed changes
Comment thread lib/internal/fs/promises.js Outdated

@legendecas legendecas Apr 16, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kylo5aby kylo5aby Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Resolved. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kylo5aby
fs: keep fs.promises.readFile read until EOF is reached
b1cdb35
@kylo5aby
kylo5aby force-pushed the fs-readfile branch from c9cbbd1 to b1cdb35 Compare April 17, 2024 08:47
legendecas
legendecas approved these changes Apr 17, 2024
View reviewed changes
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 17, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 17, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 17, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label May 11, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 11, 2024
@nodejs-github-bot
nodejs-github-bot merged commit ff7910b into nodejs:main May 11, 2024
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

Landed in ff7910b
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@orgads

orgads commented May 27, 2024

Copy link
Copy Markdown
Contributor

Can this be backported to v20?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@kylo5aby @marco-ippolito
fs: keep fs.promises.readFile read until EOF is reached
7d38c2e 
PR-URL: #52178
Fixes: #52155
Reviewed-By: Chengzhong Wu <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@kylo5aby @soophoo
fs: keep fs.promises.readFile read until EOF is reached
7e580b2 
PR-URL: nodejs#52178
Fixes: nodejs#52155
Reviewed-By: Chengzhong Wu <[email protected]>
@coderabbitai coderabbitai Bot mentioned this pull request May 19, 2026
fix: phase 1.1 — persistence & artifact boundary hardening JoseAntonioNuevo/patchloop#3
Merged
4 of 5 tasks
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

fs Issues and PRs related to file-system APIs and the fs module. 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

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@kylo5aby @H4ad @nodejs-github-bot @orgads @legendecas @BridgeAR @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.