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

[v8.x] fs: fix createReadStream(…, {end: n}) for non-seekable fds #19410

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Backport #19329 to v8.x

The test file needed to be updated.

Fixes: #19240

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]>
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. v8.x labels Mar 17, 2018
@addaleax
Copy link
Member Author

@gibfahn gibfahn self-assigned this Mar 19, 2018
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
Copy link
Contributor

landed in 5586089

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: #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]>
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.

4 participants