fix(node/fs): improve compatibility of fs.ReadStream #2653
Conversation
Actual Behavior: After reading a file once and leaving it unclosed, it cannot be read again from the beginning
| const readInterval = setInterval(() => { | ||
| if (stream) return; | ||
| if (!appended) return; | ||
|
|
||
| stream = fs.createReadStream(file, { | ||
| highWaterMark: hwm, |
There was a problem hiding this comment.
This test was failing about 10 / 30 of the time, but it seems to be a false positive.
If createReadStream() is executed before the append to the test file is completed, the "data" event will not be called and common.mustCallAtLeast() verification will fail.
It is most likely an I/O performance issue, not ReadStream. The performance of fs.writeFileSync() in Deno is about 5 or 6 times slower than in Node. (Confirmed with Deno 1.25.3 and Node.js v18.9.0 on my laptop)
To avoid the issue, I have modified the test to make sure that the append to the test file is complete before executing createReadStream().
| setTimeout(() => { | ||
| if (stream && !stream.destroyed) { | ||
| stream.on('close', () => { | ||
| assert(cur); | ||
| process.exit(); | ||
| }); | ||
| stream.destroy(); | ||
| } else { | ||
| assert(cur); | ||
| process.exit(); | ||
| }); | ||
| stream.destroy(); | ||
| } else { | ||
| process.exit(); | ||
| } | ||
| } | ||
| }, 20); |
There was a problem hiding this comment.
A small wait time has been added because common.mustCallAtLeast() verification may also fail if stream.destroy() is called and the stream is terminated in the middle.
And assert(cur) has been added to detect a case where the test terminates without ever reading any data as a failure.
|
As commented in #discussion_r972879858 and #discussion_r972880948, I have modified the contents of test-fs-read-stream-pos.js. There is one error in ci / node, but it appears to be unrelated to the changes in this PR. |
node/_tools/config.json
Outdated
| @@ -33,6 +38,7 @@ | |||
| "test-fs-append-file.js", | |||
| "test-fs-chmod-mask.js", | |||
| "test-fs-chmod.js", | |||
| "test-fs-read-stream-pos.js", | |||
There was a problem hiding this comment.
I'm thinking we should just not include this test for the time being. We can revisit it when the underlying issues are addressed.
There was a problem hiding this comment.
I understand your intention. The test has been removed. e459b0b
This is in regards to the comment on #2634 (review).
part of #911
Scope
This PR aims to address the followings:
fd,start,end, andfsoptionsOut of Scope
After examination, I'd like to transfer the following test-related implementations to later Issues and PRs, as they are difficult to address or require a large implementation outside of ReadStream.
fs.read/closemust be executed in the ReadStream. However, the default import offsin the ReadStream module leads to circular dependencies, so it cannot be implemented in the same way as Node (9305a95). Any advice on how to resolve this would be appreciated.fs.open/closeare overridden within the test.FileHandleclass forfs.promises.open. (Ref.)fs.writeFileSync()causing a false positive.child_process.spawnSyncis required within the test. (Waiting for PR 2637)child_process.execis required within the test. (Waiting for PR 2684)References
https://github.com/nodejs/node/blob/main/lib/internal/fs/streams.js
Remarks
(*1)
"bufferSize" option is present in the following tests.
The option was removed in Node v0.9.12, and the current behavior of Node's ReadStream is to ignore it.
Therefore, "bufferSize" option has not been implemented, but consider it fine.
(*2)
The following test repeatedly reads data for 90 seconds.
If no errors occur during the read, the assert is never triggered, and it seems to be the intent of the test.