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

kylo5aby · 2024-03-21T15:39:03Z

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

H4ad · 2024-03-22T16:44:37Z

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

BridgeAR

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.

BridgeAR · 2024-03-22T18:06:14Z

lib/internal/fs/promises.js

    if (bytesRead === 0 ||
        totalRead === size ||
-        (bytesRead !== buffer.length && !chunkedRead)) {
+        (bytesRead !== buffer.length && !chunkedRead && !noSize)) {


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.

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?

BridgeAR · 2024-03-22T18:11:05Z

lib/internal/fs/promises.js

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


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 · 2024-03-25T06:37:04Z

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 · 2024-03-25T07:08:45Z

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 · 2024-03-25T07:11:53Z

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

kylo5aby · 2024-04-16T08:43:24Z

could you take a look? @legendecas

legendecas · 2024-04-16T22:11:43Z

lib/internal/fs/promises.js

      return result;
    }
-
+    const writeBuffer = bytesRead !== buffer.length ?


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 ?

Resolved. Thanks!

nodejs-github-bot · 2024-04-17T22:29:51Z

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

nodejs-github-bot · 2024-05-11T15:39:22Z

Landed in ff7910b

PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

orgads · 2024-05-27T05:17:46Z

Can this be backported to v20?

PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#52178 Fixes: nodejs#52155 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

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

BridgeAR reviewed Mar 22, 2024

View reviewed changes

kylo5aby closed this Mar 25, 2024

kylo5aby reopened this Mar 25, 2024

kylo5aby force-pushed the fs-readfile branch from 212c845 to 5d35b0b Compare March 25, 2024 07:05

kylo5aby force-pushed the fs-readfile branch from 5d35b0b to c9cbbd1 Compare March 27, 2024 02:46

legendecas reviewed Apr 16, 2024

View reviewed changes

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

b1cdb35

kylo5aby force-pushed the fs-readfile branch from c9cbbd1 to b1cdb35 Compare April 17, 2024 08:47

legendecas approved these changes Apr 17, 2024

View reviewed changes

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

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

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

v22.2.0 release proposal #52971

Merged

marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024

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

51e6edc

PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

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

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

7cfdcff

PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

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>

eliph4z pushed a commit to eliph4z/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>

bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024

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

5159f46

PR-URL: nodejs#52178 Fixes: nodejs#52155 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

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

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

Uh oh!

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

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

Uh oh!

Conversation

kylo5aby commented Mar 21, 2024

Uh oh!

H4ad commented Mar 22, 2024

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Mar 22, 2024

Choose a reason for hiding this comment

Uh oh!

kylo5aby Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

BridgeAR Mar 22, 2024

Choose a reason for hiding this comment

Uh oh!

kylo5aby commented Mar 25, 2024

Uh oh!

kylo5aby commented Mar 25, 2024

Uh oh!

kylo5aby commented Mar 25, 2024

Uh oh!

kylo5aby commented Apr 16, 2024

Uh oh!

legendecas Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

kylo5aby Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 17, 2024

Uh oh!

nodejs-github-bot commented May 11, 2024

Uh oh!

orgads commented May 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants