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

fs: fix options.end of fs.ReadStream() #18121

Closed

Conversation

Projects
None yet
8 participants
@MoonBall
Copy link
Member

commented Jan 12, 2018

Fixes: #18116

options.end of fs.ReadStream does not work when options.start is undefined.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@cjihrig
Copy link
Contributor

left a comment

This needs at least one test.

lib/fs.js Outdated
@@ -2236,7 +2236,8 @@ function ReadStream(path, options) {
this.flags = options.flags === undefined ? 'r' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

this.start = options.start;
this.start = (typeof this.fd !== 'number' && options.start === undefined) ?

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 12, 2018

Contributor

I think the parens can be dropped.

@MoonBall

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2018

@cjihrig I added a test and droped the parens.

@@ -68,3 +68,16 @@ w.on('results', function(res) {
});

r.pipe(w);

const optionsEnd = 3;

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 13, 2018

Contributor

Could you write this like:

{
  // Verify that end works when start is not specified.
  const end = 3;
  const r = new FSReadable(file, { end });
  const w = new TestWriter();

  w.on('results', common.mustCall((res) => {
    assert.strictEqual(w.length, end + 1);
  }));

  r.pipe(w);
}

This comment has been minimized.

Copy link
@MoonBall

MoonBall Jan 13, 2018

Author Member

ok.

@joyeecheung
Copy link
Member

left a comment

Code changes LGTM with one suggestion.

@@ -68,3 +68,16 @@ w.on('results', function(res) {
});

r.pipe(w);

{
// Verify that end works when start is not specified.

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 13, 2018

Member

The test file here is named test-stream2-fs and it's in sequential. I think the test case added here belongs to test/parallel/test-fs-read-stream.js since it's more of a fs test, not a stream2 test. (Also I think this file can be moved to parallel? Although that should be done in another PR).

This comment has been minimized.

Copy link
@MoonBall

MoonBall Jan 14, 2018

Author Member

ok. I just moved the new test case to test/parallel/test-fs-read-stream.js.

@joyeecheung

This comment has been minimized.

@MoonBall

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2018

@joyeecheung What should I do to make the checks successful.

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

@MoonBall Those are unrelated known failures, see #18190. The CI looks green to me

@joyeecheung

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

Landed in 82bdf8f, thanks!

joyeecheung added a commit that referenced this pull request Jan 18, 2018

fs: fix options.end of fs.ReadStream()
Fixes: #18116
PR-URL: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>

@addaleax addaleax removed the author ready label Jan 18, 2018

evanlucas added a commit that referenced this pull request Jan 30, 2018

fs: fix options.end of fs.ReadStream()
Fixes: #18116
PR-URL: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

fs: fix options.end of fs.ReadStream()
Fixes: nodejs#18116
PR-URL: nodejs#18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

fs: fix options.end of fs.ReadStream()
Fixes: nodejs#18116
PR-URL: nodejs#18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>

MylesBorins added a commit that referenced this pull request Feb 27, 2018

fs: fix options.end of fs.ReadStream()
Fixes: #18116
PR-URL: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>

MylesBorins added a commit that referenced this pull request Feb 27, 2018

fs: fix options.end of fs.ReadStream()
Fixes: #18116
PR-URL: #18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>

This was referenced Feb 27, 2018

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

This seems to be causing a small regression so I think we should revert from v6.x and v8.x

/cc @gibfahn prs incoming

MylesBorins added a commit to MylesBorins/node that referenced this pull request Mar 13, 2018

Revert "fs: fix options.end of fs.ReadStream()"
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 pull request Mar 13, 2018

Revert "fs: fix options.end of fs.ReadStream()"
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 pull request Mar 13, 2018

Revert "fs: fix options.end of fs.ReadStream()"
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

addaleax added a commit to addaleax/node that referenced this pull request Mar 13, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 aligns behaviour with the native file stream mechanism
introduced in nodejs#18936 as well.

Fixes: nodejs#19240
Refs: nodejs#18121

addaleax added a commit to addaleax/node that referenced this pull request Mar 13, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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

addaleax added a commit to addaleax/node that referenced this pull request Mar 14, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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

addaleax added a commit that referenced this pull request Mar 17, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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>

addaleax added a commit to addaleax/node that referenced this pull request Mar 17, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 pull request Mar 17, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 that referenced this pull request Mar 17, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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>

MylesBorins added a commit that referenced this pull request Mar 20, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 added a commit that referenced this pull request Mar 20, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 added a commit that referenced this pull request Mar 20, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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>

FallenRiteMonk added a commit to FallenRiteMonk/node that referenced this pull request Mar 23, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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>

MylesBorins added a commit that referenced this pull request Mar 28, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 added a commit that referenced this pull request Mar 28, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 added a commit that referenced this pull request Mar 30, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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 added a commit that referenced this pull request Mar 30, 2018

fs: fix `createReadStream(…, {end: n})` for non-seekable fds
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>

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

fs: fix options.end of fs.ReadStream()
Fixes: nodejs#18116
PR-URL: nodejs#18121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.