Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v6.13.1 createReadStream() invalid seek #19240

Closed
wankdanker opened this issue Mar 8, 2018 · 18 comments
Closed

v6.13.1 createReadStream() invalid seek #19240

wankdanker opened this issue Mar 8, 2018 · 18 comments
Assignees
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@wankdanker
Copy link
Contributor

It believe commit 82bdf8f (fs: fix options.end of fs.ReadStream()) broke my code for creating a read stream. It looks to me like this.start is now forced to the value 0 whereas previously it was allowed to be undefined. I'm guessing since it is 0 it now forces a seek operation which breaks my use case.

In v6.13.1, this code:

var fs = require('fs');
var reader = fs.createReadStream('/dev/ttyS0');

results in this error:

Error: ESPIPE: invalid seek, read
    at Error (native)

The above code in v6.13.0 does not create that error.

In v6.13.0, this code:

var fs = require('fs');
var reader = fs.createReadStream('/dev/ttyS0', { start : 0 });

results in this error:

Error: ESPIPE: invalid seek, read
    at Error (native)
@MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins self-assigned this Mar 12, 2018
@MoonBall
Copy link
Member

@wankdanker I'm sorry for it. But I think current code is ok since we considered that if fd is specified and start is omitted or undefined, fs.createReadStream() reads sequentially from the current file position.

I think your problem will be solved by getting the fd of '/dev/ttyS0'. You can try the below code, but I'm not sure it's going to succeed.

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

util.promisify(fs.open)('/dev/ttyS0', 'r').then(fd => {
  const reader = fs.createReadStream(undefined, { fd: fd });
  reader.on('data', console.log);
});

@wankdanker
Copy link
Contributor Author

wankdanker commented Mar 13, 2018

@MoonBall Thanks for your reply. I've had this code in production since at least as far back as v0.10.x without this breaking. Just seems odd to me to see this changed all of a sudden in v6.13.1.

@MoonBall
Copy link
Member

MoonBall commented Mar 13, 2018

@wankdanker I suppose, the cause is the /dev/ttyS0 file doesn't support seeking. But I didn't find an official document to explain it.

@MylesBorins
Copy link
Contributor

@MoonBall a semverpatch should never change behavior, we may want to revert this on all LTS lines and mark it as semver-major for 10.x. Even if the behavior is incorrect we might not want to change it in LTS releases if people are relying on it.

/cc @nodejs/lts

@MoonBall
Copy link
Member

MoonBall commented Mar 13, 2018

@MylesBorins reverting it is ok to me. #18121 just fixed a small issue and I didn't consider this problem.

@MylesBorins
Copy link
Contributor

@wankdanker I've opened a reversion against v6.x and am in the process of building a test release. Once that release is built would you be able to run it and verify in the PR that this fixes your problem

@addaleax addaleax added fs Issues and PRs related to the fs subsystem / file system. and removed v6.x labels Mar 13, 2018
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 13, 2018

@wankdanker I've tried to reproduce on OSX

running

var fs = require('fs');
var reader = fs.createReadStream('/dev/ttyS0');

results in the error you report for v6.13.1 on various version of v6.x including v6.13.0 and v6.12.3

I'm going to test on a cloud ubuntu box and see if I can repro.

Can you tell us a bit more about your setup

edit: looks like there is no way for me to test this in the cloud rn

@addaleax
Copy link
Member

@MylesBorins Hope it doesn’t bother you that I tend to chime in on these kinds of issues :) This problem is not specific to v6.x, so I think we should take care of it in master too.

Reverting would be an option, but I think we could also fix the underlying issue here, maybe in a better way than before:

diff in the fold
diff --git a/lib/fs.js b/lib/fs.js
index f890e431d2a9..8324ff0542a9 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -1967,8 +1967,7 @@ function ReadStream(path, options) {
   this.flags = options.flags === undefined ? 'r' : options.flags;
   this.mode = options.mode === undefined ? 0o666 : options.mode;
 
-  this.start = typeof this.fd !== 'number' && options.start === undefined ?
-    0 : options.start;
+  this.start = options.start;
   this.end = options.end;
   this.autoClose = options.autoClose === undefined ? true : options.autoClose;
   this.pos = undefined;
@@ -1993,6 +1992,12 @@ function ReadStream(path, options) {
     this.pos = this.start;
   }
 
+  // Backwards compatibility: Make sure `end` is a number regardless of `start`.
+  // TODO(addaleax): Make the above typecheck not depend on `start` instead.
+  // (That is a semver-major change).
+  if (typeof this.end !== 'number')
+    this.end = Infinity;
+
   if (typeof this.fd !== 'number')
     this.open();
 
@@ -2047,6 +2052,8 @@ ReadStream.prototype._read = function(n) {
 
   if (this.pos !== undefined)
     toRead = Math.min(this.end - this.pos + 1, toRead);
+  else
+    toRead = Math.min(this.end - this.bytesRead + 1, toRead);
 
   // already read everything we were supposed to read!
   // treat as EOF.
diff --git a/test/parallel/test-fs-read-stream.js b/test/parallel/test-fs-read-stream.js
index 7fc7a0d56bce..75b2fe3d14b8 100644
--- a/test/parallel/test-fs-read-stream.js
+++ b/test/parallel/test-fs-read-stream.js
@@ -21,7 +21,9 @@
 
 'use strict';
 const common = require('../common');
+const tmpdir = require('../common/tmpdir');
 
+const child_process = require('child_process');
 const assert = require('assert');
 const fs = require('fs');
 const fixtures = require('../common/fixtures');
@@ -178,6 +180,31 @@ common.expectsError(
   }));
 }
 
+{
+  // Verify that end works when start is not specified, and we do not try to
+  // use positioned reads. This makes sure that this keeps working for
+  // non-seekable file descriptors.
+  tmpdir.refresh();
+  const filename = `${tmpdir.path}/foo.pipe`;
+  const mkfifoResult = child_process.spawnSync('mkfifo', [filename]);
+  if (!mkfifoResult.error) {
+    child_process.exec(`echo "xyz foobar" > '${filename}'`);
+    const stream = new fs.createReadStream(filename, { end: 1 });
+    stream.data = '';
+
+    stream.on('data', function(chunk) {
+      stream.data += chunk;
+    });
+
+    stream.on('end', common.mustCall(function() {
+      assert.strictEqual('xy', stream.data);
+      fs.unlinkSync(filename);
+    }));
+  } else {
+    common.printSkipMessage('mkfifo not available');
+  }
+}
+
 {
   // pause and then resume immediately.
   const pauseRes = fs.createReadStream(rangeFile);

Will open a PR shortly if that’s okay.

MylesBorins added a commit to MylesBorins/node that referenced this issue Mar 13, 2018
This reverts commit df038ad.

Some people were relying on the behavior of this.start being able to be
undefined, whereas after the change it is being set to 0.

Fixes: nodejs#19240
Refs: nodejs#18121
MylesBorins added a commit to MylesBorins/node that referenced this issue Mar 13, 2018
This reverts commit b343cb6.

Some people were relying on the behavior of this.start being able to be
undefined, whereas after the change it is being set to 0.

Refs: nodejs#19240
Refs: nodejs#18121
@MylesBorins
Copy link
Contributor

Here is a link to the 8.x revert #19328

@addaleax I am absolutely thrilled when you chime in. I push for a revert only because I lack time + context to solve this in a more elegant way. I 100% support moving forward with a better solution if we can fix it properly!

@wankdanker
Copy link
Contributor Author

Hi @MylesBorins!

Sure! We've got several machines running Ubuntu 14.04 and 16.04 that have Mettler Toledo scales connected to the serial port. It's been a long time since I wrote my application and worked with the scale protocol, but I believe until we write to the serial port asking the scale for a measurement, there is nothing to read from the serial port.

Let me know if you have any other questions. And I'll run the test build whenever that is available. Thanks!

@wankdanker
Copy link
Contributor Author

@MylesBorins The test build worked for me. Thank you.

@addaleax
Copy link
Member

@wankdanker I don’t know if trying it out is an option for you, but I’m pretty confident that #19329 fixes this issue without undoing the original fix, so there’s a decent chance we’ll be going with that.

If you want to & can try that, that would be awesome. I suppose that if it’s much more convenient for you, Myles or somebody else from the release team could also produce another test build with it.

@wankdanker
Copy link
Contributor Author

@addaleax If I get a chance, I'll give it a go. It would definitely easier if someone generated a test build, but I wouldn't want to inconvenience anyone. 😄

addaleax added a commit to addaleax/node that referenced this issue Mar 14, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in nodejs#18936 as well.

Fixes: nodejs#19240
Refs: nodejs#18121
@gibfahn
Copy link
Member

gibfahn commented Mar 14, 2018

Hope it doesn’t bother you that I tend to chime in on these kinds of issues :)

@addaleax I am absolutely thrilled when you chime in. I push for a revert only because I lack time + context to solve this in a more elegant way. I 100% support moving forward with a better solution if we can fix it properly!

💯 to what Myles said (from an 8.x perspective). Extremely grateful for anyone who is willing to get involved and work out a better solution than just reverting.

@fivdi
Copy link

fivdi commented Mar 17, 2018

I'm seeing the same issue with Node v8.10.0 when using read streams to read from FIFO special files (named pipes) on Linux. With Node v8.9.4 all is ok.

addaleax added a commit to addaleax/node that referenced this issue Mar 17, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in nodejs#18936 as well.

PR-URL: nodejs#19329
Fixes: nodejs#19240
Refs: nodejs#18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
addaleax added a commit to addaleax/node that referenced this issue Mar 17, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in nodejs#18936 as well.

PR-URL: nodejs#19329
Fixes: nodejs#19240
Refs: nodejs#18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
@addaleax
Copy link
Member

Opened backports for the fixes in #19410 and #19411.

@nodejs/lts Given the comments here, do we want to consider fast-tracking those?

addaleax added a commit that referenced this issue Mar 17, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
@gibfahn
Copy link
Member

gibfahn commented Mar 19, 2018

@nodejs/lts Given the comments here, do we want to consider fast-tracking those?

I think given that we were willing to revert in the next release, then we should fast-track this (given that we're not reverting).

MylesBorins pushed a commit that referenced this issue Mar 20, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

Backport-PR-URL: #19411
PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

Backport-PR-URL: #19410
PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
dfunckt added a commit to balena-io-modules/node-docker-delta that referenced this issue Mar 28, 2018
Early Node 8 versions have a bug that force a seek upon creation of a read stream. The workaround is to pass the file descriptor directly. See: nodejs/node#19240

Change-Type: patch
dfunckt added a commit to balena-io-modules/node-docker-delta that referenced this issue Mar 28, 2018
Early Node 8 versions have a bug that force a seek upon creation of a read stream. The workaround is to pass the file descriptor directly. See: nodejs/node#19240

Change-Type: patch
MylesBorins pushed a commit that referenced this issue Mar 28, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

Backport-PR-URL: #19411
PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
MylesBorins pushed a commit that referenced this issue Mar 28, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

Backport-PR-URL: #19410
PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
MylesBorins pushed a commit that referenced this issue Mar 30, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

Backport-PR-URL: #19411
PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
MylesBorins pushed a commit that referenced this issue Mar 30, 2018
82bdf8f fixed an issue by silently modifying the `start`
option for the case when only `end` is passed, in order to perform
reads from a specified range in the file.

However, that approach does not work for non-seekable files, since
a numeric `start` option means that positioned reads will be used
to read data from the file.

This patch fixes that, and instead ends reading after a specified
size by adjusting the read buffer size.

This way we avoid re-introducing the bug that 82bdf8f fixed,
and align behaviour with the native file stream mechanism
introduced in #18936 as well.

Backport-PR-URL: #19410
PR-URL: #19329
Fixes: #19240
Refs: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chen Gang <gangc.cxy@foxmail.com>
pimterry added a commit to balena-io-experimental/sense-snake that referenced this issue Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants