Skip to content

Commit

Permalink
stream: increase MAX_HWM
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ronag committed Oct 20, 2019
1 parent c8df5cf commit d05b777
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 21 deletions.
25 changes: 4 additions & 21 deletions lib/_stream_readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,25 +374,6 @@ Readable.prototype.setEncoding = function(enc) {
return this;
};

// Don't raise the hwm > 8MB
const MAX_HWM = 0x800000;
function computeNewHighWaterMark(n) {
if (n >= MAX_HWM) {
n = MAX_HWM;
} else {
// Get the next highest power of 2 to prevent increasing hwm excessively in
// tiny amounts
n--;
n |= n >>> 1;
n |= n >>> 2;
n |= n >>> 4;
n |= n >>> 8;
n |= n >>> 16;
n++;
}
return n;
}

// This function is designed to be inlinable, so please take care when making
// changes to the function body.
function howMuchToRead(n, state) {
Expand Down Expand Up @@ -425,9 +406,11 @@ Readable.prototype.read = function(n) {
const state = this._readableState;
const nOrig = n;

// If we're asking for more than the current hwm, then raise the hwm.
// If we're asking for more than the current hwm, then raise the hwm to
// the next highest power of 2 to prevent increasing hwm excessively in
// tiny amounts.
if (n > state.highWaterMark)
state.highWaterMark = computeNewHighWaterMark(n);
state.highWaterMark = Math.pow(2, Math.ceil(Math.log(n) / Math.log(2)));

if (n !== 0)
state.emittedReadable = false;
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-readable-large-hwm.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';
const common = require('../common');
const { Readable } = require('stream');

// Make sure that readable completes
// even when reading larger buffer.
const bufferSize = 10 * 1024 * 1024;
let n = 0;
const r = new Readable({
read() {
// Try to fill readable buffer piece by piece.
r.push(Buffer.alloc(bufferSize / 10));

if (n++ > 10) {
r.push(null);
}
}
});

r.on('readable', () => {
while (true) {
const ret = r.read(bufferSize);
if (ret === null)
break;
}
});
r.on('end', common.mustCall());

0 comments on commit d05b777

Please sign in to comment.