fs: fix options.end of fs.ReadStream() by MoonBall · Pull Request #18121 · nodejs/node

MoonBall · 2018-01-12T16:20:30Z

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

Fixes: nodejs#18116

cjihrig

This needs at least one test.

cjihrig · 2018-01-12T17:31:46Z

lib/fs.js

  this.mode = options.mode === undefined ? 0o666 : options.mode;

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


I think the parens can be dropped.

Add a test. Fixes: nodejs#18116

MoonBall · 2018-01-13T03:41:58Z

@cjihrig I added a test and droped the parens.

cjihrig · 2018-01-13T16:53:39Z

test/sequential/test-stream2-fs.js


 r.pipe(w);
+
+const optionsEnd = 3;


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

joyeecheung

Code changes LGTM with one suggestion.

joyeecheung · 2018-01-13T23:15:08Z

test/sequential/test-stream2-fs.js

 r.pipe(w);
+
+{
+  // Verify that end works when start is not specified.


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

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

joyeecheung · 2018-01-17T17:31:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12584/

MoonBall · 2018-01-17T18:57:41Z

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

joyeecheung · 2018-01-18T00:11:27Z

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

joyeecheung · 2018-01-18T00:53:07Z

Landed in 82bdf8f, thanks!

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 · 2018-03-13T18:49:11Z

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

/cc @gibfahn prs incoming

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

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

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

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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

036b29b

Fixes: nodejs#18116

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

jasnell approved these changes Jan 12, 2018

View reviewed changes

cjihrig reviewed Jan 12, 2018

View reviewed changes

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

1981fbf

Add a test. Fixes: nodejs#18116

cjihrig approved these changes Jan 13, 2018

View reviewed changes

Modify code style of the test

609404f

joyeecheung approved these changes Jan 13, 2018

View reviewed changes

Move the test cast to test-fs-read-stream.js

3723635

starkwang approved these changes Jan 15, 2018

View reviewed changes

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 closed this Jan 18, 2018

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 mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added land-on-v6.x labels Feb 27, 2018

This was referenced Feb 27, 2018

v6.13.1 proposal #19025

Closed

v6.13.1 proposal #19027

Merged

MoonBall mentioned this pull request Mar 13, 2018

v6.13.1 createReadStream() invalid seek #19240

Closed

MylesBorins added dont-land-on-v6.x and removed land-on-v6.x labels Mar 13, 2018

MylesBorins mentioned this pull request Mar 13, 2018

[6.x] Revert "fs: fix options.end of fs.ReadStream()" #19326

Closed

MylesBorins mentioned this pull request Mar 13, 2018

[8.x] Revert "fs: fix options.end of fs.ReadStream()" #19328

Closed

addaleax mentioned this pull request Mar 13, 2018

fs: fix createReadStream(…, {end: n}) for non-seekable fds #19329

Closed

3 tasks

Uh oh!

Conversation

MoonBall commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Jan 12, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBall commented Jan 13, 2018

Uh oh!

cjihrig Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBall Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 13, 2018

Choose a reason for hiding this comment

Uh oh!

MoonBall Jan 14, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 17, 2018

Uh oh!

MoonBall commented Jan 17, 2018

Uh oh!

joyeecheung commented Jan 18, 2018

Uh oh!

joyeecheung commented Jan 18, 2018

Uh oh!

MylesBorins commented Mar 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

MoonBall commented Jan 12, 2018 •

edited

Loading