fs: readFile function adds the chunkSize option #41647

mawaregetsuka · 2022-01-22T12:03:26Z

According to the discussion in pr 41436 adding a configurable parameter is probably the best solution

lib/fs.js

Trott · 2022-01-22T19:09:31Z

doc/api/fs.md

@@ -3205,11 +3205,15 @@ changes:
  * `encoding` {string|null} **Default:** `null`
  * `flag` {string} See [support of file system `flags`][]. **Default:** `'r'`.
  * `signal` {AbortSignal} allows aborting an in-progress readFile
+  * `kPerRead` {integer} The number of kb per read, -1 means no limit.


The options are in alphabetical order. Let's keep them that way by moving this up one line.

Suggested change

* `kPerRead` {integer} The number of kb per read, -1 means no limit.

* `kPerRead` {integer} The number of KB per read. Use `-1` for no limit.

Is there a reason why we're using kilobytes instead of just bytes? Using the latter is more sensible and more flexible IMO.

Also, I think a name like chunkSize or similar would be a better name, especially since the /^k[A-Z]/ prefix is used internally in code for private/internal field/symbol names.

Trott · 2022-01-22T19:12:37Z

This will need tests.

mscdex · 2022-01-22T21:56:44Z

lib/internal/fs/utils.js

+
+  if (defaultOptions.kPerRead) {
+    if (options.kPerRead === undefined) {
+      options.kPerRead = defaultOptions.kPerRead;


Isn't this mutating the end user's options object?

What I mean by this is that I believe options is the object passed in by the user. We shouldn't overwrite/set values on the object they pass in. AFAICT the only time options is not an end user's object is when they don't supply one.

Co-authored-by: Mohammed Keyvanzadeh <[email protected]>

mawaregetsuka · 2022-01-22T22:41:17Z

I've solved the alphabetical order problem @Trott
About KB vs Byte I don't think users need that kind of precision, in fact, the existing configuration is also in kilobytes
About name I haven't consider about naming conventions yet maybe chunkSize is great
About

Isn't this mutating the end user's options object?

I didn't catch your meaning maybe I need an example
@mscdex

mscdex · 2022-01-23T00:58:29Z

About KB vs Byte I don't think users need that kind of precision

I think using bytes would make it more consistent with other storage-based input values used in node core. For example: highWaterMark in streams, Buffer.alloc*(), etc. I'm not aware of any commonly used APIs in core that use units larger than bytes. Besides, it's not as if the range of a double will be insufficient for representing a byte-based chunk size that anyone would ever want to use.

If someone wants to use something less than 1KB, they have to use awkward fractional values and even then because of the floating point representation they might not be able to get the exact value they want.

in fact, the existing configuration is also in kilobytes

The value currently being used (kReadFileBufferLength) is measured in bytes.

mawaregetsuka · 2022-01-23T08:32:02Z

Okay, I'll change it to byte later
In this function if I don't use options then need to add an extra parameter, is that reaally necessary?
options changes after input should be common with nodejs

mawaregetsuka · 2022-01-25T16:43:57Z

It looks like the absence of a period would cause the test/parallel/test-cluster-primary-error test to time out, but why? I cannot reproduce timeout in CI locally.

jasnell · 2022-01-28T22:41:35Z

doc/api/fs.md

@@ -3202,6 +3202,8 @@ changes:

 * `path` {string|Buffer|URL|integer} filename or file descriptor
 * `options` {Object|string}
+  * `bPerRead` {integer} The number of bytes per read. Use `-1` for no limit.


s/bPerRead/chunkSize

aduh95 · 2022-03-12T00:26:39Z

@mawaregetsuka are you still interested to implement this? Can you address the comments above regarding the name of the option please?
We'd also need to add an entry to the YAML list in the docs.

node/doc/api/fs.md

Lines 3315 to 3320 in 80bfb9b

    
           changes: 
        
             - version: REPLACEME 
        
               pr-url: https://github.com/nodejs/node/pull/41678 
        
               description: Passing an invalid callback to the `callback` argument 
        
                            now throws `ERR_INVALID_ARG_TYPE` instead of 
        
                            `ERR_INVALID_CALLBACK`.

mawaregetsuka · 2022-03-14T05:40:37Z

Sure,I'll finish it as soon as possible
@aduh95

aduh95

I think we would need a test for this new option.

Also, could you please rebase to avoid the merge commit? Merge commits tend to break our tooling unfortunately. Let me know if you need help for that.

doc/api/fs.md

mawaregetsuka · 2022-03-16T13:23:22Z

@aduh95
Thanks for your quick reply,I've considered adding test cases but I'm not sure if I want to add them in this PR.
about merge I'll make sure to add --rebase option every time, thank you for your notice.

aduh95 · 2022-03-16T13:28:02Z

about merge I'll make sure to add --rebase option every time

To be clear, it doesn't have to be every time, it's only need when fixing a git conflict.

I've considered adding test cases but I'm not sure if I want to add them in this PR

I'm sure we do :) if we add a feature with no tests, we cannot guarantee it will still be working next time we change something in the source code (which happens often in a project the size of Node.js). Let us know if you need help writing the tests of course.

mawaregetsuka · 2022-03-16T16:32:50Z

This is my first time writing tests for nodejs and I'd love to get some suggestions.

aduh95

A few suggestions to improve the testing. Should we add a similar option to fs.promises.readFile?

aduh95 · 2022-03-16T18:09:28Z

test/parallel/test-fs-readfile.js

+  for (const e of fileInfo) {
+    fs.readFile(e.name, { chunkSize: e.chunkSize },
+                common.mustCall((err, buf) => {
+                  assert.deepStrictEqual(buf, e.contents);
+                }));
+  }


First I think we want to make sure invalid values are not silently ignored, but is raising a useful error. Then we want to make sure the file is read as expected for valid values of chunkSize:

Suggested change

for (const e of fileInfo) {

fs.readFile(e.name, { chunkSize: e.chunkSize },

common.mustCall((err, buf) => {

assert.deepStrictEqual(buf, e.contents);

}));

}

['', Symbol(), () => {}, 1n, true, false, [], {}].forEach((chunkSize) => {

assert.throws(

() => fs.readFile(fileInfo[0].name, { chunkSize }, common.mustNotCall()),

{ code: 'ERR_INVALID_ARG_TYPE' }

);

});

[-1, 0, -Infinity, Infinity, NaN].forEach((chunkSize) => {

assert.throws(

() => fs.readFile(fileInfo[0].name, { chunkSize }, common.mustNotCall()),

{ code: 'ERR_INVALID_RANGE' }

);

});

[null, undefined, 1, 64, Number.MAX_SAFE_INTEGER].forEach((chunkSize) => {

fs.readFile(fileInfo[0].name, { chunkSize: e.chunkSize },

common.mustSucceed((buf) => {

assert.deepStrictEqual(buf, fileInfo[0].contents);

}));

});

aduh95 · 2022-03-16T18:09:31Z

test/parallel/test-fs-readfile.js

  { name: path.join(tmpdir.path, `${prefix}-1K.txt`),
-    len: 1024 },
+    len: 1024, chunkSize: 64 },
  { name: path.join(tmpdir.path, `${prefix}-64K.txt`),
-    len: 64 * 1024 },
+    len: 64 * 1024, chunkSize: -64 },
  { name: path.join(tmpdir.path, `${prefix}-64KLessOne.txt`),
-    len: (64 * 1024) - 1 },
+    len: (64 * 1024) - 1, chunkSize: 'string' },
  { name: path.join(tmpdir.path, `${prefix}-1M.txt`),
-    len: 1 * 1024 * 1024 },
+    len: 1 * 1024 * 1024, chunkSize: 0 },
  { name: path.join(tmpdir.path, `${prefix}-1MPlusOne.txt`),
-    len: (1 * 1024 * 1024) + 1 },
+    len: (1 * 1024 * 1024) + 1, chunkSize: -1 },


I suggest reverting those changes.

mawaregetsuka · 2022-03-18T17:06:16Z

I've improved the tests but there are still a few problems
1 The TypeError about Symbol() and BigInt(1n) does not have error code, should we improve this in other PR?
2 chunkSize is a parameter passed into libuv and I have no idea how to check if it works in the test phase.

aduh95 · 2023-09-20T08:17:48Z

@nodejs/fs

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 Jan 22, 2022

fs: readFile function adds the kPerRead option

081f1c3

mawaregetsuka force-pushed the fs branch from cee1b25 to 081f1c3 Compare January 22, 2022 12:43

VoltrexKeyva reviewed Jan 22, 2022

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

Trott reviewed Jan 22, 2022

View reviewed changes

mscdex reviewed Jan 22, 2022

View reviewed changes

mawaregetsuka and others added 2 commits January 23, 2022 05:57

Apply suggestions from code review

f91c99d

Co-authored-by: Mohammed Keyvanzadeh <[email protected]>

fs: keep alphabetical order

a41479d

mawaregetsuka added 3 commits January 23, 2022 17:45

change the unit to byte

66e74e5

trigger GitHub actions

f5f20de

add a period

4c16365

jasnell reviewed Jan 28, 2022

View reviewed changes

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 12, 2022

mawaregetsuka and others added 2 commits March 16, 2022 00:34

change bPerRead to chunkSize

fc32447

Merge branch 'master' into fs

6a94fb5

aduh95 reviewed Mar 15, 2022

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

change version number to REPLACEME

e2a99de

add a test

339f181

aduh95 reviewed Mar 16, 2022

View reviewed changes

improve the test

69b8678

fix test-fs-options-immutable.js fail

6a53d19

tniessen changed the title ~~fs: readFile function adds the kPerRead option~~ fs: readFile function adds the chunkSize option Mar 19, 2022

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: readFile function adds the chunkSize option #41647

fs: readFile function adds the chunkSize option #41647

mawaregetsuka commented Jan 22, 2022

Trott Jan 22, 2022

Trott Jan 22, 2022

mscdex Jan 22, 2022 •

edited

Loading

Trott commented Jan 22, 2022

mscdex Jan 22, 2022

mscdex Jan 23, 2022

mawaregetsuka commented Jan 22, 2022

mscdex commented Jan 23, 2022 •

edited

Loading

mawaregetsuka commented Jan 23, 2022

mawaregetsuka commented Jan 25, 2022

jasnell Jan 28, 2022

aduh95 commented Mar 12, 2022

mawaregetsuka commented Mar 14, 2022

aduh95 left a comment

mawaregetsuka commented Mar 16, 2022

aduh95 commented Mar 16, 2022 •

edited

Loading

mawaregetsuka commented Mar 16, 2022

aduh95 left a comment

aduh95 Mar 16, 2022

aduh95 Mar 16, 2022

mawaregetsuka commented Mar 18, 2022

aduh95 commented Sep 20, 2023

	* `kPerRead` {integer} The number of kb per read, -1 means no limit.
	* `kPerRead` {integer} The number of KB per read. Use `-1` for no limit.

-  for (const e of fileInfo) {
-    fs.readFile(e.name, { chunkSize: e.chunkSize },
-                common.mustCall((err, buf) => {
-                  assert.deepStrictEqual(buf, e.contents);
-                }));
-  }
+  ['', Symbol(), () => {}, 1n, true, false, [], {}].forEach((chunkSize) => {
+    assert.throws(
+      () => fs.readFile(fileInfo[0].name, { chunkSize }, common.mustNotCall()),
+      { code: 'ERR_INVALID_ARG_TYPE' }
+    );
+  });
+  [-1, 0, -Infinity, Infinity, NaN].forEach((chunkSize) => {
+    assert.throws(
+      () => fs.readFile(fileInfo[0].name, { chunkSize }, common.mustNotCall()),
+      { code: 'ERR_INVALID_RANGE' }
+    );
+  });
+  [null, undefined, 1, 64, Number.MAX_SAFE_INTEGER].forEach((chunkSize) => {
+    fs.readFile(fileInfo[0].name, { chunkSize: e.chunkSize },
+                common.mustSucceed((buf) => {
+                  assert.deepStrictEqual(buf, fileInfo[0].contents);
+                }));
+  });

fs: readFile function adds the chunkSize option #41647

Are you sure you want to change the base?

fs: readFile function adds the chunkSize option #41647

Conversation

mawaregetsuka commented Jan 22, 2022

Trott Jan 22, 2022

Choose a reason for hiding this comment

Trott Jan 22, 2022

Choose a reason for hiding this comment

mscdex Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

Trott commented Jan 22, 2022

mscdex Jan 22, 2022

Choose a reason for hiding this comment

mscdex Jan 23, 2022

Choose a reason for hiding this comment

mawaregetsuka commented Jan 22, 2022

mscdex commented Jan 23, 2022 • edited Loading

mawaregetsuka commented Jan 23, 2022

mawaregetsuka commented Jan 25, 2022

jasnell Jan 28, 2022

Choose a reason for hiding this comment

aduh95 commented Mar 12, 2022

mawaregetsuka commented Mar 14, 2022

aduh95 left a comment

Choose a reason for hiding this comment

mawaregetsuka commented Mar 16, 2022

aduh95 commented Mar 16, 2022 • edited Loading

mawaregetsuka commented Mar 16, 2022

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 Mar 16, 2022

Choose a reason for hiding this comment

aduh95 Mar 16, 2022

Choose a reason for hiding this comment

mawaregetsuka commented Mar 18, 2022

aduh95 commented Sep 20, 2023

mscdex Jan 22, 2022 •

edited

Loading

mscdex commented Jan 23, 2022 •

edited

Loading

aduh95 commented Mar 16, 2022 •

edited

Loading