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

thefourtheye · 2017-01-14T19:17:46Z

This reverts commit 4444e73.

Looks like #9699 was landed without a CI Run. It breaks three tests in my
Mac OS.

/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release -J \
    addons doctool inspector known_issues message pseudo-tty parallel sequential
=== release test-fs-readfile-pipe ===
Path: parallel/test-fs-readfile-pipe
assert.js:381
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: Command failed: cat "/Users/thefourtheye/git/node/test/parallel/test-fs-readfile-pipe.js" | "/Users/thefourtheye/git/node/out/Release/node" "/Users/thefourtheye/git/node/test/parallel/test-fs-readfile-pipe.js" child
assert.js:381
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: ESPIPE: invalid seek, read

    at ChildProcess.exithandler (child_process.js:212:12)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:192:7)
    at maybeClose (internal/child_process.js:886:16)
    at Socket.<anonymous> (internal/child_process.js:334:11)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:189:7)
    at Pipe._handle.close [as _onclose] (net.js:501:12)
Command: out/Release/node /Users/thefourtheye/git/node/test/parallel/test-fs-readfile-pipe.js
=== release test-fs-readfile-pipe-large ===
Path: parallel/test-fs-readfile-pipe-large
assert.js:381
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: Command failed: cat /Users/thefourtheye/git/node/test/tmp.3/readfile_pipe_large_test.txt | "/Users/thefourtheye/git/node/out/Release/node" "/Users/thefourtheye/git/node/test/parallel/test-fs-readfile-pipe-large.js" child
assert.js:381
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: ESPIPE: invalid seek, read
cat: stdout: Broken pipe

    at ChildProcess.exithandler (child_process.js:212:12)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:192:7)
    at maybeClose (internal/child_process.js:886:16)
    at Socket.<anonymous> (internal/child_process.js:334:11)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:189:7)
    at Pipe._handle.close [as _onclose] (net.js:501:12)
Command: out/Release/node /Users/thefourtheye/git/node/test/parallel/test-fs-readfile-pipe-large.js
=== release test-fs-readfilesync-pipe-large ===
Path: parallel/test-fs-readfilesync-pipe-large
assert.js:381
assert.ifError = function ifError(err) { if (err) throw err; };
                                                  ^

Error: Command failed: cat /Users/thefourtheye/git/node/test/tmp.5/readfilesync_pipe_large_test.txt | "/Users/thefourtheye/git/node/out/Release/node" "/Users/thefourtheye/git/node/test/parallel/test-fs-readfilesync-pipe-large.js" child
fs.js:581
  return binding.read(fd, buffer, offset, length, position);
                 ^

Error: ESPIPE: invalid seek, read
    at Object.fs.readSync (fs.js:581:18)
    at tryReadSync (fs.js:454:20)
    at Object.fs.readFileSync (fs.js:491:19)
    at Object.<anonymous> (/Users/thefourtheye/git/node/test/parallel/test-fs-readfilesync-pipe-large.js:16:27)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
cat: stdout: Broken pipe

    at ChildProcess.exithandler (child_process.js:212:12)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:192:7)
    at maybeClose (internal/child_process.js:886:16)
    at Socket.<anonymous> (internal/child_process.js:334:11)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:189:7)
    at Pipe._handle.close [as _onclose] (net.js:501:12)
Command: out/Release/node /Users/thefourtheye/git/node/test/parallel/test-fs-readfilesync-pipe-large.js

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

fs

cc @seishun

This reverts commit 4444e73.

seishun · 2017-01-14T19:39:05Z

Which tests fail for you?

addaleax

I can reproduce these failures on master on Linux

LGTM

addaleax · 2017-01-14T19:48:05Z

I’m landing this to un-break master… it’s just a revert so we shouldn’t need to wait for CI

@seishun The PR description was edited here to include the relevant output of make test, that’s probably helpful :)

addaleax · 2017-01-14T19:51:35Z

Landed in 66f09be

This reverts commit 4444e73. PR-URL: #10809 Reviewed-By: Anna Henningsen <anna@addaleax.net>

seishun · 2017-01-14T21:21:59Z

Ok, I see why it fails. The tests call fs.readFile on /dev/stdin which doesn't have a beginning, so trying to seek to 0 results in an error. I can think of several options:

Use the old behavior when fs.readFile is called on a path. var fd = fs.openSync('/dev/stdin', 'r'); fs.readFileSync(fd); would still fail though.
Use the old behavior on '/dev/stdin' and other things where you can't seek (any TTY, pipe, socket, etc.), as well as fds that were opened from them.
Disallow readFile on any fds mentioned above. (hey, the docs say "reads the entire contents of a file" which kinda doesn't make sense for fd opened from '/dev/stdin')
Give up and don't change any code, instead change the documentation to state that fs.readFile reads from the current file position to the end when called on an fd.

cc @nodejs/platform-macos @nodejs/collaborators

addaleax · 2017-01-14T21:46:10Z

anything else that might need a special case?

stdin shouldn’t be special in any way; You should get ESPIPE errors when trying to read from any TTY, pipe, socket, etc.

"reads the entire contents of a file" which kinda doesn't make sense for /dev/stdin

There might be a reasonable interpretation what that means then talking about stdin; namely, open the file and read until EOF is hit. What pretty much doesn’t make sense here is reading from a given offset.

(This isn’t MacOS-specific, by the way; it should affect all POSIX systems, at least.)

seishun · 2017-01-14T21:56:37Z

stdin shouldn’t be special in any way; You should get ESPIPE errors when trying to read from any TTY, pipe, socket, etc.

Right, in that case it would have to use the old behavior for all TTYs, pipes and sockets.

There might be a reasonable interpretation what that means then talking about stdin; namely, open the file and read until EOF is hit.

Right, if you're using readFile with a path, then /dev/stdin isn't any different from regular files. But if you have an fd which has already been read from, then one would expect the "entire" to also include the data that was already read.

addaleax · 2017-01-14T22:05:56Z

Right, in that case it would have to use the old behavior for all TTYs, pipes and sockets.

We could probably reasonably do special-casing based on whether the fd refers to a regular file or not, yeah.

But if you have an fd which has already been read from, then one would expect the "entire" to also include the data that was already read.

Okay, this is obviously just my personal impression, but I wouldn’t actually expect that to happen? Maybe it’s just me, but I’d definitely find it weird for the same data to be read twice from an fd.

seishun · 2017-01-15T09:19:35Z

Okay, this is obviously just my personal impression, but I wouldn’t actually expect that to happen? Maybe it’s just me, but I’d definitely find it weird for the same data to be read twice from an fd.

Are you talking about "non-regular" fds or all fds? In the latter case, the current behavior is what you want, and I'd be fine with it, but the docs need to be clarified then.

joyeecheung · 2017-01-15T09:33:44Z

Should we add an item to the PR template checklist about running CI for the last commit before the PR lands? (and other things like number of LGTMs, 48-hour wait, .etc)

jasnell · 2017-01-15T16:43:47Z

@joyeecheung: That likely wouldn't help much. It's just something that those of us landing PRs need to remember to check for.

Thank you @addaleax for landing the revert.

addaleax · 2017-01-15T21:54:48Z

Are you talking about "non-regular" fds or all fds? In the latter case, the current behavior is what you want, and I'd be fine with it, but the docs need to be clarified then.

I definitely can see arguments for both perspective, but yeah, what I had in mind was having the current behaviour for all file types.

This reverts commit 4444e73. PR-URL: nodejs#10809 Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

22b1b36

This reverts commit 4444e73.

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. dont-land-on-v7.x labels Jan 14, 2017

addaleax approved these changes Jan 14, 2017

View reviewed changes

addaleax closed this Jan 14, 2017

thefourtheye deleted the revert-9699 branch January 14, 2017 19:51

addaleax pushed a commit that referenced this pull request Jan 14, 2017

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

66f09be

This reverts commit 4444e73. PR-URL: #10809 Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 dont-land-on-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 pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017

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

5dd54a6

This reverts commit 4444e73. PR-URL: nodejs#10809 Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

thefourtheye mentioned this pull request Oct 11, 2018

fs: behaviour of readFile and writeFile with file descriptors #23433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thefourtheye commented Jan 14, 2017 •

edited

Loading

seishun commented Jan 14, 2017

addaleax left a comment

addaleax commented Jan 14, 2017

addaleax commented Jan 14, 2017

seishun commented Jan 14, 2017 •

edited

Loading

addaleax commented Jan 14, 2017

seishun commented Jan 14, 2017

addaleax commented Jan 14, 2017

seishun commented Jan 15, 2017

joyeecheung commented Jan 15, 2017

jasnell commented Jan 15, 2017

addaleax commented Jan 15, 2017

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

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

Conversation

thefourtheye commented Jan 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

seishun commented Jan 14, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Jan 14, 2017

addaleax commented Jan 14, 2017

seishun commented Jan 14, 2017 • edited Loading

addaleax commented Jan 14, 2017

seishun commented Jan 14, 2017

addaleax commented Jan 14, 2017

seishun commented Jan 15, 2017

joyeecheung commented Jan 15, 2017

jasnell commented Jan 15, 2017

addaleax commented Jan 15, 2017

thefourtheye commented Jan 14, 2017 •

edited

Loading

seishun commented Jan 14, 2017 •

edited

Loading