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

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Mar 13, 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.

Fixes: #19240
Refs: #18121

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Mar 13, 2018
@addaleax
Copy link
Member Author

addaleax commented Mar 13, 2018

@addaleax
Copy link
Member Author

addaleax commented Mar 13, 2018

The changes to lib/fs.js land cleanly on v8.x and v6.x, but it looks like the test file parts would require explicit backporting (neighbouring line conflicts/different common tmpdir interface).

@addaleax
Copy link
Member Author

@nodejs/build I think the original CI attempt here left the Windows tmpdirs in a bogus state… could you maybe refresh those dirs (make sure they can be rimraf’ed) and start one more CI for this?

@joaocgreis
Copy link
Member

This PR creates a fifo on Windows, even if it does not use it. However, that fifo cannot be removed by tmpdir.refresh(), causing other tests to fail (I believe this boils down to an issue with libuv, I'm still looking into that). For now, can you make it so that the fifo is not even created for Windows?

@MoonBall
Copy link
Member

MoonBall commented Mar 14, 2018

There is probably a inconsistent behavior. The end option of fs.createReadStream('non-seekable-file', { end: 3 }) doesn't work as a normal file.

Assuming the content of a file is:

abcdefg

If the file is a normal file, then fs.createReadStream('non-seekable-file', { end: 3 }) will get 'abcd'. If the file is non-seekable file and the current file position is 2, then fs. createReadStream('non-seekable-file', { end: 3 }) will get 'cdef'.

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

If the file is non-seekable file and the current file position is 2

@MoonBall No; non-seekable files have no such concept as a “current position” (or a “content”). That’s the reason why this is failing in the first place.

For now, can you make it so that the fifo is not even created for Windows?

@joaocgreis Done!

CI: https://ci.nodejs.org/job/node-test-commit/16898/

@MoonBall
Copy link
Member

MoonBall commented Mar 14, 2018

@addaleax LGTM.
But there is a small change about the end option. Please see the code below.

const fs = require('fs');

const fd = fs.openSync('a.txt', 'r'); // a.txt's content is `123456789`
const buf = Buffer.alloc(3);
fs.read(fd, buf, 0, 2);

const readable = fs.createReadStream(null, { fd: fd, end: 3 });
readable.on('data', data => console.log(data.toString()));

Executing this code in current stable Node.js, the output is 3456789, but now the output is 3456.

@addaleax
Copy link
Member Author

Executing this code in current stable Node.js, the output is 3456789, but now the output is 3456.

You’re right, that’s a change. I can’t really see how 3456789 would be an expected output if you explicitly pass end: 3, so I think that’s okay?

@jasnell jasnell requested a review from mcollina March 14, 2018 11:47
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Would need docs. Otherwise LGTM

@addaleax
Copy link
Member Author

@jasnell This is a bugfix, it doesn’t introduce a new option but it rather makes a specific combination of existing ones work… what would you like to see in the documentation?

@jasnell
Copy link
Member

jasnell commented Mar 14, 2018

oh right right... sorry :-) (I had mistakenly read fix in the title as add ... :-) ..)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@MoonBall
Copy link
Member

You’re right, that’s a change. I can’t really see how 3456789 would be an expected output if you explicitly pass end: 3, so I think that’s okay?

that's okay.

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

@addaleax
Copy link
Member Author

Landed in 2e37618

@addaleax addaleax closed this Mar 17, 2018
@addaleax addaleax deleted the 19240 branch March 17, 2018 11:37
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
@addaleax addaleax added backported-to-v6.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lts-watch-v6.x labels Mar 17, 2018
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
@targos targos mentioned this pull request Mar 18, 2018
@joaocgreis
Copy link
Member

Just for reference, libuv fix for the issue I mentioned above: libuv/libuv#1774

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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chen Gang <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 13, 2018
@MylesBorins MylesBorins mentioned this pull request May 2, 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 this pull request may close these issues.

v6.13.1 createReadStream() invalid seek
6 participants