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

attempting to use a readstream to read a specific size of bytes no longer works properly with Node 10 #29933

Closed
BrianFreedman opened this issue Oct 11, 2019 · 3 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.

Comments

@BrianFreedman
Copy link

BrianFreedman commented Oct 11, 2019

Here's my OS info:
Edition: Windows 10
Version: 1903
OS Build: 18362.356

Here's the output from Node 10.16.3 x64

c:\code\temp>node -v
v10.16.3

c:\code\temp>node readBytes.js
size > bufferSize : true
null segment: true
null segment: true

Here's the output from Node 8.15.1 x64

c:\code\temp>node -v
v8.15.1

c:\code\temp>node readBytes.js
size > bufferSize : true
null segment: true
null segment: true
null segment: false
null segment: true
null segment: false
null segment: true
null segment: false
null segment: true
done! 3 chunks

here's the contents of readBytes.js which illustrates the problem

const fs = require('fs');
const filepath = 'C:/code/temp/21_MB_File';
const bufferSize = 10 * 1024 * 1024;

const chunks = [];
const stats = fs.statSync(filepath);
console.log(`size > bufferSize : ${stats.size > bufferSize}`);

const readableStream = fs.createReadStream(filepath);
let segmentIndex = 1;
readableStream.on('readable', () => {
	let segment = readableStream.read(bufferSize);
	console.log(`null segment: ${segment === null}`);
	while (segment !== null) {
		chunks.push({ index: segmentIndex++, body: segment });
		segment = readableStream.read(bufferSize);
		console.log(`null segment: ${segment === null}`);
	}
});
readableStream.on('end', () => {
	console.log(`done! ${chunks.length} chunks`);
});
@addaleax addaleax added stream Issues and PRs related to the stream subsystem. fs Issues and PRs related to the fs subsystem / file system. labels Oct 11, 2019
@addaleax
Copy link
Member

/cc @ronag

@addaleax addaleax added the confirmed-bug Issues with confirmed bugs. label Oct 11, 2019
@ronag

This comment has been minimized.

@ronag ronag mentioned this issue Oct 11, 2019
4 tasks
@ronag
Copy link
Member

ronag commented Oct 11, 2019

Fixed in #29938

ronag added a commit to nxtedition/node that referenced this issue Oct 20, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: nodejs#29933
@Trott Trott closed this as completed in f8c069f Nov 12, 2019
MylesBorins pushed a commit that referenced this issue Nov 17, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this issue Dec 1, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 17, 2019
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this issue Jan 7, 2020
MAX_HWM was added in 9208c89 where the highwatermark was changed to
always increase in steps of highest power of 2 to prevent increasing
hwm excessivly in tiny amounts.

Why a limit was added on the highwatermark is unclear but breaks
existing usage where a larger read size is used. The invariant for
read(n) is that a buffer of size n is always returned. Considering
a maximum ceiling on the buffer size breaks this invariant.

This PR significantly increases the limit to make it less likely to
break the previous invariant and also documents the limit.

Fixes: #29933

PR-URL: #29938
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants