FS promises: handle long files reading #19643

aduh95 · 2018-03-27T19:01:09Z

Fixes #19443

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

I am not sure about the testing, should I include a very large file on which we can test? On my local machine, I test using the scripts described in the issue.

TimothyGu · 2018-03-27T20:04:57Z

lib/fs/promises.js

+    totalRead += bytesRead;
+    if (bytesRead > 0)
+      chunks.push(buffer.slice(0, bytesRead));
+  } while (totalRead < size);


size cannot be used as a reliable indicator of when the loop should exit since the file size might change while readFileHandle() is running. Maybe what we could do is using bytesRead === chunkSize as the exit condition.

TimothyGu · 2018-03-27T20:05:45Z

Thanks for fixing this! In hindsight, the logic errors seem fairly obvious. One comment.

mscdex · 2018-03-28T00:25:13Z

lib/fs/promises.js

-    if (totalRead > 0)
-      chunks.push(buffer.slice(0, totalRead));
-  } while (totalRead === chunkSize);
+    const chunks = [];


This indentation looks off. There's no way this passed make test.

You're right, I forgot to re-run the tests after TimothyGu change request. My bad!

TimothyGu · 2018-03-28T00:47:52Z

lib/fs/promises.js

+      await read(filehandle, buf, 0, chunkSize, totalRead);
+    totalRead += bytesRead;
+    if (bytesRead !== chunkSize)
+      endOfFile = true;


nit: replace these two lines with endOfFile = bytesRead !== chunkSize;

Isn't that very less efficient to set the variable at each iteration?

About 5% slower according to the benchmarks I just ran, so "very less" is a bit overstating, still it's not nothing.

Interesting, I assumed it would be faster due to lack of branching (at least it would be in a language like C). Oh well.

TimothyGu · 2018-03-28T00:53:09Z

should I include a very large file on which we can test?

How large does it have to be? From the logic itself it seems as if a file larger than 16384 bytes is enough, or am I missing something?

aduh95 · 2018-03-28T00:56:15Z

@TimothyGu Indeed, 16385 bytes would be enough to check that issue. Should I create such a file in the fixatures folder?

TimothyGu · 2018-03-28T00:58:05Z

@aduh95 Yeah that'd work. Or you could find a file in the source tree that's already that big (like AUTHORS).

joyeecheung · 2018-03-28T05:14:36Z

test/parallel/test-fs-promises-read-file.js

+const { readFile } = require('fs/promises');
+const fixtures = require('../common/fixtures');
+
+const fn = fixtures.path('../../AUTHORS');


This seems to defeat the purpose of fixtures. Maybe just create a file with

const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); const fn = path.join(tmpdir.path, 'large-file'); // Write a file and then read it again using `readFile`

Or, create a file with a known hash in fixtures.

Good idea, this way I don't need the crypto module anymore for the test

TimothyGu · 2018-03-28T16:07:19Z

lib/fs/promises.js

  let totalRead = 0;
+  let endOfFile;


Initialize this to false.

Why though?

Better to keep one variable the same type (undefined vs. boolean), when it is used as a boolean anyway.

Fair enough

TimothyGu · 2018-03-28T17:04:34Z

test/parallel/test-fs-promises-read-file.js

+// Creating large buffer with random content
+const buffer = Buffer.from(
+  Array.apply(null, { length: 16834 * 2 })
+    .map(Number.call, Math.random)


nit: would just .map(Math.random) work?

joyeecheung · 2018-03-29T03:14:29Z

test/parallel/test-fs-promises-read-file.js

+common.crashOnUnhandledRejection();
+
+// Creating large buffer with random content
+const buffer = Buffer.from(


You can generate this buffer with crypto.randomBytes(16834 * 2) actually, but the current method LGTM anyway.

BridgeAR · 2018-04-09T17:28:29Z

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

aduh95 · 2018-04-10T15:45:54Z

I am a bit confused about the CI result...
Does that mean this PR fails on some architectures? Where can I find info to solve it? (Sorry, I am newbie on the subject)

jasnell · 2018-04-14T20:30:44Z

Lots of red in the CI results that appear to be build bot failures... trying again https://ci.nodejs.org/job/node-test-pull-request/14293/

TimothyGu · 2018-04-15T23:09:16Z

One last CI run: https://ci.nodejs.org/job/node-test-pull-request/14315/

TimothyGu · 2018-04-16T01:17:33Z

CI passes (after a rerun on AIX). Landed in 11819c7.

PR-URL: #19643 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2018-05-01T13:34:07Z

This is based on a semver-major as far as I know, so I marked them as do not land on the other release lines.

TimothyGu reviewed Mar 27, 2018

View reviewed changes

TimothyGu added fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises. labels Mar 27, 2018

TimothyGu requested a review from jasnell March 27, 2018 20:05

aduh95 force-pushed the fix-fs-promise-readfile branch 2 times, most recently from 49bf54c to 9a9ea11 Compare March 27, 2018 20:48

jasnell approved these changes Mar 27, 2018

View reviewed changes

mscdex reviewed Mar 28, 2018

View reviewed changes

aduh95 force-pushed the fix-fs-promise-readfile branch from 9a9ea11 to a450a5e Compare March 28, 2018 00:46

TimothyGu approved these changes Mar 28, 2018

View reviewed changes

aduh95 force-pushed the fix-fs-promise-readfile branch from a450a5e to 614d2f5 Compare March 28, 2018 01:41

TimothyGu approved these changes Mar 28, 2018

View reviewed changes

joyeecheung reviewed Mar 28, 2018

View reviewed changes

aduh95 force-pushed the fix-fs-promise-readfile branch from 614d2f5 to 9d04eb8 Compare March 28, 2018 15:39

TimothyGu reviewed Mar 28, 2018

View reviewed changes

aduh95 force-pushed the fix-fs-promise-readfile branch from 9d04eb8 to bbe41ed Compare March 28, 2018 16:53

TimothyGu approved these changes Mar 28, 2018

View reviewed changes

promises: handle long files reading

f083126

aduh95 force-pushed the fix-fs-promise-readfile branch from bbe41ed to f083126 Compare March 28, 2018 17:16

joyeecheung reviewed Mar 29, 2018

View reviewed changes

joyeecheung approved these changes Mar 29, 2018

View reviewed changes

benjamingr approved these changes Apr 9, 2018

View reviewed changes

BridgeAR approved these changes Apr 9, 2018

View reviewed changes

aduh95 closed this Apr 15, 2018

aduh95 deleted the fix-fs-promise-readfile branch April 15, 2018 19:20

aduh95 restored the fix-fs-promise-readfile branch April 15, 2018 19:21

aduh95 reopened this Apr 15, 2018

TimothyGu added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2018

TimothyGu closed this Apr 16, 2018

aduh95 mentioned this pull request Apr 16, 2018

FS promises does not work well with big files #19443

Closed

BridgeAR added dont-land-on-v4.x labels May 1, 2018

aduh95 deleted the fix-fs-promise-readfile branch May 1, 2018 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FS promises: handle long files reading #19643

FS promises: handle long files reading #19643

aduh95 commented Mar 27, 2018 •

edited

Loading

TimothyGu Mar 27, 2018

TimothyGu commented Mar 27, 2018

mscdex Mar 28, 2018 •

edited

Loading

aduh95 Mar 28, 2018

TimothyGu Mar 28, 2018

aduh95 Mar 28, 2018

aduh95 Mar 28, 2018

TimothyGu Mar 28, 2018

TimothyGu commented Mar 28, 2018

aduh95 commented Mar 28, 2018

TimothyGu commented Mar 28, 2018

joyeecheung Mar 28, 2018

aduh95 Mar 28, 2018

TimothyGu Mar 28, 2018

aduh95 Mar 28, 2018

TimothyGu Mar 28, 2018

aduh95 Mar 28, 2018

TimothyGu Mar 28, 2018

aduh95 Mar 28, 2018

joyeecheung Mar 29, 2018

BridgeAR commented Apr 9, 2018

aduh95 commented Apr 10, 2018

jasnell commented Apr 14, 2018

TimothyGu commented Apr 15, 2018

TimothyGu commented Apr 16, 2018

BridgeAR commented May 1, 2018

FS promises: handle long files reading #19643

FS promises: handle long files reading #19643

Conversation

aduh95 commented Mar 27, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

TimothyGu commented Mar 27, 2018

mscdex Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 28, 2018

aduh95 commented Mar 28, 2018

TimothyGu commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Apr 9, 2018

aduh95 commented Apr 10, 2018

jasnell commented Apr 14, 2018

TimothyGu commented Apr 15, 2018

TimothyGu commented Apr 16, 2018

BridgeAR commented May 1, 2018

aduh95 commented Mar 27, 2018 •

edited

Loading

mscdex Mar 28, 2018 •

edited

Loading