fs: make sure readFile[Sync] reads from the beginning #9699

seishun · 2016-11-19T17:47:22Z

Checklist

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

Affected core subsystem(s)

fs

Description of change

Currently, fs.readFile[Sync] starts reading from the current file position, which can be non-zero if the fd was previously read or written from/to. This conflicts with the documentation which states that it "reads the entire contents of a file".

Fixes: #9671

Provisional testcase for fs.readFile:

const assert = require('assert');
const fs = require('fs');

const filleLength = 2;
const path = 'test-read-file';
const fd = fs.openSync(path, 'w+');

fs.writeSync(fd, Buffer.alloc(filleLength, 1), 0, filleLength);

fs.readFile(fd, (err, buf) => {
  if (err) throw err;
  assert.strictEqual(buf.length, filleLength);
});

For fs.readFileSync:

'use strict';

const assert = require('assert');
const fs = require('fs');

const fileLength = 2;
const path = 'test-read-file';
const fd = fs.openSync(path, 'w+');

fs.writeSync(fd, Buffer.alloc(fileLength, 1), 0, fileLength);

assert.strictEqual(fs.readFileSync(fd).length, fileLength);

vsemozhetbyt · 2016-11-19T18:02:57Z

It seems you will be asked to add some tests. Here are the possible ones even without writing and additional files to read:

'use strict';
const common = require('../common');
const assert = require('assert');
const fs = require('fs');

const fd = fs.openSync(__filename, 'r');
const fileLength = fs.readFileSync(fd).length;

assert.strictEqual(fileLength, fs.readFileSync(fd).length);

'use strict';
const common = require('../common');
const assert = require('assert');
const fs = require('fs');

const fd = fs.openSync(__filename, 'r');
const fileLength = fs.readFileSync(fd).length;

fs.readFile(fd, common.mustCall((err, buf) => {
  if (err) throw err;
  assert.strictEqual(fileLength, buf.length);
}));

sam-github · 2016-11-21T16:42:07Z

To the two tests proposed above I would add an fs.stat() call to verify the length is the disk size of the file.

jasnell · 2016-12-05T22:50:00Z

Marking this semver-major to be cautious. Adding tests is definitely a good thing

bnoordhuis · 2016-12-05T23:20:36Z

I'd also add a test that checks it works with file descriptors opened in 'a+' mode (append + read/write.)

Fixes: #9671

seishun · 2016-12-06T13:08:25Z

Added test.

@sam-github I think the test should test just one thing, namely that doing readFile[Sync] repeatedly returns the same data every time.

@jasnell IMO it shouldn't be semver-major, it's a bug fix. I mean there's a possibility that someone out there is relying on it, but the the same could be said about any bug.

cjihrig

The test looks like it contains duplicated logic. You could probably extract it into a function that is called twice.

cjihrig · 2016-12-06T14:35:08Z

test/parallel/test-fs-readfile-fd-offset.js

+const fileLengthR = fs.readFileSync(fdr).length;
+const fileLengthA = fs.readFileSync(fda).length;
+
+// reading again should result in the same length


Can you capitalize and punctuate this comment.

cjihrig · 2016-12-06T14:35:33Z

test/parallel/test-fs-readfile-fd-offset.js

+assert.strictEqual(fileLengthA, fs.readFileSync(fda).length);
+
+fs.readFile(fdr, common.mustCall((err, buf) => {
+  if (err) throw err;


assert.ifError(err);

cjihrig · 2016-12-06T14:35:52Z

test/parallel/test-fs-readfile-fd-offset.js

+}));
+
+fs.readFile(fda, common.mustCall((err, buf) => {
+  if (err) throw err;


Same thing here.

seishun · 2016-12-06T15:06:53Z

@cjihrig Agreed, done. I think this way is better than a function.

sam-github · 2016-12-06T15:14:01Z

@sam-github I think the test should test just one thing, namely that doing readFile[Sync] repeatedly returns the same data every time.

@seishun Is this test not intended to test the API contract of readFile? If so, the API contract is not that it returns the same data every time, but that it returns the entire file contents every time - the test as written doesn't do that. Calling stat will at least guarantee that what you read from disk is the same size as what is on disk. Right now if a bug was introduced so that only the first block of a data was returned when reading from a fd, your test looks like it would pass. To be more thorough, you could readFile, change the data on disk, and readFile again... but since that isn't a suspected regression its overkill at this moment.

sam-github · 2016-12-06T15:15:43Z

I don't think this is semver-major. Its hard to see how the current behaviour would be depended on. Possible - sure, like any bug can be depended on, but not likely. Marking semver-major would block back-porting.

seishun · 2016-12-06T15:19:58Z

@sam-github Alright, I guess you're right, although now the test depends on fs.stat working correctly. What do others think?

sam-github · 2016-12-06T15:39:15Z

The test is always going to depend on some other fs APIs if it needs a fixture, and this does. You avoided having to create the file to read by using your own js file, which is clever, but at the price of not knowing what the file's contents are, so you no longer know if you've read the whole file contents. I don't think its likely that readFile will read incorrect data if the size of what it read is at least identical to the size on disk, but some kind of assertion that there is at least a relationship between the data you are trying to read and the data you did read is important.

bnoordhuis · 2016-12-06T16:17:59Z

test/parallel/test-fs-readfile-fd-offset.js

+const fileLength = fs.statSync(__filename).size;
+
+['r', 'a+'].forEach(mode => {
+  const fd = fs.openSync(__filename, mode);


I would use a file in common.tmpDir for a+ because __filename is not going to work when the file is owned by a different user.

Isn't that used for new files? We need an already existing file here.

Yes, just create a file upfront with fs.writeFileSync().

seishun · 2016-12-06T16:51:29Z

@bnoordhuis like this?

bnoordhuis

LGTM when the style nits are fixed.

bnoordhuis · 2016-12-06T19:08:02Z

test/parallel/test-fs-readfile-fd-offset.js

+const path = require('path');
+
+const filename = path.join(common.tmpDir, 'readfile.txt');
+const dataExpected = new Array(100).join('a');


'a'.repeat(100)?

Yeah, I'm still not up to speed with ES6. In my defense, I copied from test-fs-readfilesync-pipe-large.js.

bnoordhuis · 2016-12-06T19:08:27Z

test/parallel/test-fs-readfile-fd-offset.js

+fs.writeFileSync(filename, dataExpected);
+const fileLength = dataExpected.length;
+
+['r', 'a+'].forEach(mode => {


Isn't the linter complaining that it should be (mode) => {?

bnoordhuis · 2016-12-06T19:10:10Z

test/parallel/test-fs-readfile-fd-offset.js

+
+['r', 'a+'].forEach(mode => {
+  const fd = fs.openSync(filename, mode);
+  assert.strictEqual(fileLength, fs.readFileSync(fd).length);


Tiny nit: it should be assert.strictEqual(actual, expected) here and below.

seishun · 2016-12-06T19:48:27Z

@bnoordhuis nits are fixed.

jasnell · 2016-12-06T20:41:10Z

@nodejs/ctc ... we need some discussion on whether this change is semver-major or not.

trevnorris · 2016-12-09T22:08:16Z

@jasnell I'd say a patch. Current behavior contradicts official documentation, and this PR aligns the two.

jasnell · 2016-12-09T22:13:22Z

Works for me

seishun · 2016-12-17T15:24:01Z

@jasnell so you're okay with removing semver-major?

seishun · 2017-01-08T16:45:49Z

@nodejs/collaborators Merging in a week from now if there are no objections.

It would previously read from the current file position, which can be non-zero if the `fd` has been read from or written to. This contradicts the documentation which states that it "reads the entire contents of a file". PR-URL: nodejs#9699 Fixes: nodejs#9671 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

targos · 2017-01-28T11:02:09Z

Since this has been reverted, should we add all the dont-land-on labels?

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 19, 2016

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 5, 2016

seishun added 2 commits December 6, 2016 13:59

fs: make sure readFile[Sync] reads from the beginning

9e44700

Fixes: #9671

add test

818e7ee

cjihrig requested changes Dec 6, 2016

View reviewed changes

improve tests

97069d5

sam-github added the lts-agenda label Dec 6, 2016

use stat

f53bf47

bnoordhuis requested changes Dec 6, 2016

View reviewed changes

use temp file

07d8bc4

bnoordhuis approved these changes Dec 6, 2016

View reviewed changes

nits

f001cf9

bnoordhuis approved these changes Dec 6, 2016

View reviewed changes

jasnell added the ctc-review label Dec 6, 2016

cjihrig approved these changes Dec 6, 2016

View reviewed changes

trevnorris mentioned this pull request Dec 12, 2016

Change in fs.statSync behavior between 7.1.0 and 7.2.0 #10242

Closed

seishun removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 8, 2017

sam-github approved these changes Jan 9, 2017

View reviewed changes

seishun merged commit 4444e73 into nodejs:master Jan 14, 2017

thefourtheye mentioned this pull request Jan 14, 2017

Revert "fs: ensure readFile[Sync] reads from the beginning" #10809

Closed

2 tasks

addaleax mentioned this pull request Jan 14, 2017

readFile(fd) does not read the whole file (at least on Windows, possibly other platforms) #9671

Closed

MylesBorins added lts-watch-v4.x labels Jan 16, 2017

seishun mentioned this pull request Jan 17, 2017

doc: clarify the behavior of readFile with fd #10853

Closed

2 tasks

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

Trott removed the ctc-review label Jan 31, 2017

evanlucas added dont-land-on-v4.x labels Jan 31, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

thisconnect mentioned this pull request Jun 9, 2017

'fs' can not read data if same FD is used for write and readFile #13572

Closed

MylesBorins removed lts-watch-v4.x labels Nov 14, 2017

seishun deleted the readfile-offset branch October 12, 2018 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: make sure readFile[Sync] reads from the beginning #9699

fs: make sure readFile[Sync] reads from the beginning #9699

seishun commented Nov 19, 2016 •

edited

Loading

vsemozhetbyt commented Nov 19, 2016 •

edited

Loading

sam-github commented Nov 21, 2016

jasnell commented Dec 5, 2016

bnoordhuis commented Dec 5, 2016

seishun commented Dec 6, 2016

cjihrig left a comment

cjihrig Dec 6, 2016

cjihrig Dec 6, 2016

cjihrig Dec 6, 2016

seishun commented Dec 6, 2016

sam-github commented Dec 6, 2016

sam-github commented Dec 6, 2016

seishun commented Dec 6, 2016

sam-github commented Dec 6, 2016

bnoordhuis Dec 6, 2016

seishun Dec 6, 2016

bnoordhuis Dec 6, 2016

seishun commented Dec 6, 2016

bnoordhuis left a comment

bnoordhuis Dec 6, 2016

seishun Dec 6, 2016

bnoordhuis Dec 6, 2016

bnoordhuis Dec 6, 2016

seishun commented Dec 6, 2016

jasnell commented Dec 6, 2016

trevnorris commented Dec 9, 2016

jasnell commented Dec 9, 2016

seishun commented Dec 17, 2016

seishun commented Jan 8, 2017

targos commented Jan 28, 2017

fs: make sure readFile[Sync] reads from the beginning #9699

fs: make sure readFile[Sync] reads from the beginning #9699

Conversation

seishun commented Nov 19, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

vsemozhetbyt commented Nov 19, 2016 • edited Loading

sam-github commented Nov 21, 2016

jasnell commented Dec 5, 2016

bnoordhuis commented Dec 5, 2016

seishun commented Dec 6, 2016

cjihrig left a 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

seishun commented Dec 6, 2016

sam-github commented Dec 6, 2016

sam-github commented Dec 6, 2016

seishun commented Dec 6, 2016

sam-github commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun commented Dec 6, 2016

bnoordhuis left a 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

seishun commented Dec 6, 2016

jasnell commented Dec 6, 2016

trevnorris commented Dec 9, 2016

jasnell commented Dec 9, 2016

seishun commented Dec 17, 2016

seishun commented Jan 8, 2017

targos commented Jan 28, 2017

seishun commented Nov 19, 2016 •

edited

Loading

vsemozhetbyt commented Nov 19, 2016 •

edited

Loading