fs: keep fs.promises.readFile read until EOF is reached #52178
Conversation
nodejs-github-bot
added
fs
kylo5aby
force-pushed
the
fs-readfile
branch
2 times, most recently
from
7e6ce28 to
212c845
Compare
|
The PR looks great, can you add a test just to ensure the issue is fixed? |
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
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.
| if (bytesRead === 0 || | ||
| totalRead === size || | ||
| (bytesRead !== buffer.length && !chunkedRead)) { | ||
| (bytesRead !== buffer.length && !chunkedRead && !noSize)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| result += decoder.write(noSize && bytesRead !== kReadFileUnknownBufferLength ? | ||
| buffer.subarray(0, bytesRead) : buffer); |
There was a problem hiding this comment.
| 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); |
|
Thanks for the reminder :) |
|
could you take a look? @legendecas |
lib/internal/fs/promises.js
Outdated
| return result; | ||
| } | ||
|
|
||
| const writeBuffer = bytesRead !== buffer.length ? |
There was a problem hiding this comment.
If I understand correctly, this is not a buffer for a "write" operation. Instead, it is a result of "read".
| const writeBuffer = bytesRead !== buffer.length ? | |
| const readBuffer = bytesRead !== buffer.length ? |
github-actions
bot
removed
the
request-ci
aduh95
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in ff7910b |
PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <[email protected]>
|
Can this be backported to v20? |
PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: #52178 Fixes: #52155 Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: nodejs#52178 Fixes: nodejs#52155 Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: nodejs#52178 Fixes: nodejs#52155 Reviewed-By: Chengzhong Wu <[email protected]>
7 participants
Fixes: #52155