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

MoonBall
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 12, 2018
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the parens can be dropped.

@MoonBall
Copy link
Member Author

@cjihrig I added a test and droped the parens.

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

r.pipe(w);

const optionsEnd = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

ok.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2018
@joyeecheung
Copy link
Member

@MoonBall
Copy link
Member Author

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

@joyeecheung
Copy link
Member

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

@joyeecheung
Copy link
Member

Landed in 82bdf8f, thanks!

joyeecheung pushed a commit that referenced this pull request Jan 18, 2018
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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
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 pushed a commit that referenced this pull request Feb 27, 2018
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 pushed a commit that referenced this pull request Feb 27, 2018
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
Copy link
Contributor

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
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
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 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
addaleax added a commit that referenced this pull request 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>
addaleax added a commit to addaleax/node that referenced this pull request 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 pull request 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 that referenced this pull request 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>
MylesBorins pushed a commit that referenced this pull request 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 pull request 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 pull request 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>
MylesBorins pushed a commit that referenced this pull request 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 pull request 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 pull request 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 pull request 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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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