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

Which tests fail for you?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reproduce these failures on master on Linux

LGTM

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 :)

This reverts commit 4444e73.

PR-URL: #10809
Reviewed-By: Anna Henningsen <anna@addaleax.net>

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:

1. 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.
2. 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.
3. 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')
4. 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

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.)

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.

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.

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.

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)

@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.

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>