From d05b777fbbd0f2f219d1065330079529ee56b076 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 20 Oct 2019 10:13:57 +0200 Subject: [PATCH 1/7] stream: increase MAX_HWM 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: https://github.com/nodejs/node/issues/29933 --- lib/_stream_readable.js | 25 ++++------------------ test/parallel/test-readable-large-hwm.js | 27 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-readable-large-hwm.js diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 5d2300cf891bca..ba5b9b6986c3a9 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -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) { @@ -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; diff --git a/test/parallel/test-readable-large-hwm.js b/test/parallel/test-readable-large-hwm.js new file mode 100644 index 00000000000000..d5bf25bc0e61c1 --- /dev/null +++ b/test/parallel/test-readable-large-hwm.js @@ -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()); From f873beef6d0ae63b45c2126d19fbbe621183c674 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 20 Oct 2019 11:13:49 +0200 Subject: [PATCH 2/7] fixup: added range check --- doc/api/stream.md | 2 ++ lib/_stream_readable.js | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index ef0bbb502735d3..a787882b380d52 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1084,6 +1084,8 @@ buffer will be returned. If the `size` argument is not specified, all of the data contained in the internal buffer will be returned. +The `size` argument must be less than or equal to `Number.MAX_SAFE_INTEGER`. + The `readable.read()` method should only be called on `Readable` streams operating in paused mode. In flowing mode, `readable.read()` is called automatically until the internal buffer is fully drained. diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index ba5b9b6986c3a9..b633bc0ee594b4 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -41,7 +41,8 @@ const { ERR_INVALID_ARG_TYPE, ERR_STREAM_PUSH_AFTER_EOF, ERR_METHOD_NOT_IMPLEMENTED, - ERR_STREAM_UNSHIFT_AFTER_END_EVENT + ERR_STREAM_UNSHIFT_AFTER_END_EVENT, + ERR_VALUE_OUT_OF_RANGE } = require('internal/errors').codes; // Lazy loaded to improve the startup performance. @@ -406,6 +407,10 @@ Readable.prototype.read = function(n) { const state = this._readableState; const nOrig = n; + if (n > Number.MAX_SAFE_INTEGER) { + throw new ERR_VALUE_OUT_OF_RANGE('n'); + } + // 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. From 7539c6ff9878f080af7a4ba92380564bd7fa90ab Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 20 Oct 2019 11:17:21 +0200 Subject: [PATCH 3/7] fixup --- doc/api/stream.md | 2 +- lib/_stream_readable.js | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index a787882b380d52..26616e7f34cf32 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1084,7 +1084,7 @@ buffer will be returned. If the `size` argument is not specified, all of the data contained in the internal buffer will be returned. -The `size` argument must be less than or equal to `Number.MAX_SAFE_INTEGER`. +The `size` argument must be less than or equal to 2^31. The `readable.read()` method should only be called on `Readable` streams operating in paused mode. In flowing mode, `readable.read()` is called diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index b633bc0ee594b4..6f563c00567919 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -45,6 +45,8 @@ const { ERR_VALUE_OUT_OF_RANGE } = require('internal/errors').codes; +const MAX_HWM = Math.pow(2, 31); + // Lazy loaded to improve the startup performance. let StringDecoder; let createReadableStreamAsyncIterator; @@ -407,7 +409,7 @@ Readable.prototype.read = function(n) { const state = this._readableState; const nOrig = n; - if (n > Number.MAX_SAFE_INTEGER) { + if (n > MAX_HWM) { throw new ERR_VALUE_OUT_OF_RANGE('n'); } @@ -415,7 +417,10 @@ Readable.prototype.read = function(n) { // the next highest power of 2 to prevent increasing hwm excessively in // tiny amounts. if (n > state.highWaterMark) - state.highWaterMark = Math.pow(2, Math.ceil(Math.log(n) / Math.log(2))); + state.highWaterMark = Math.min( + MAX_HWM, + Math.pow(2, Math.ceil(Math.log(n) / Math.log(2))) + ); if (n !== 0) state.emittedReadable = false; From a51e4044258cd3d033b645fbbcda83a82eb4b4d0 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 20 Oct 2019 11:19:14 +0200 Subject: [PATCH 4/7] fixup --- doc/api/stream.md | 2 +- lib/_stream_readable.js | 38 +++++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 26616e7f34cf32..9a2ba37c34f527 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1084,7 +1084,7 @@ buffer will be returned. If the `size` argument is not specified, all of the data contained in the internal buffer will be returned. -The `size` argument must be less than or equal to 2^31. +The `size` argument must be less than or equal to 2^30. The `readable.read()` method should only be called on `Readable` streams operating in paused mode. In flowing mode, `readable.read()` is called diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index 6f563c00567919..cd09be923c04dd 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -41,12 +41,9 @@ const { ERR_INVALID_ARG_TYPE, ERR_STREAM_PUSH_AFTER_EOF, ERR_METHOD_NOT_IMPLEMENTED, - ERR_STREAM_UNSHIFT_AFTER_END_EVENT, - ERR_VALUE_OUT_OF_RANGE + ERR_STREAM_UNSHIFT_AFTER_END_EVENT } = require('internal/errors').codes; -const MAX_HWM = Math.pow(2, 31); - // Lazy loaded to improve the startup performance. let StringDecoder; let createReadableStreamAsyncIterator; @@ -377,6 +374,26 @@ Readable.prototype.setEncoding = function(enc) { return this; }; +// Don't raise the hwm > 1GB +const MAX_HWM = Math.pow(2, 30); +function computeNewHighWaterMark(n) { + if (n >= MAX_HWM) { + // TODO(ronag): Throw ERR_VALUE_OUT_OF_RANGE. + 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) { @@ -409,18 +426,9 @@ Readable.prototype.read = function(n) { const state = this._readableState; const nOrig = n; - if (n > MAX_HWM) { - throw new ERR_VALUE_OUT_OF_RANGE('n'); - } - - // 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 we're asking for more than the current hwm, then raise the hwm. if (n > state.highWaterMark) - state.highWaterMark = Math.min( - MAX_HWM, - Math.pow(2, Math.ceil(Math.log(n) / Math.log(2))) - ); + state.highWaterMark = computeNewHighWaterMark(n); if (n !== 0) state.emittedReadable = false; From 98b770246f166aa95fe49c06f8e07f4e5d76ee4d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 20 Oct 2019 11:34:35 +0200 Subject: [PATCH 5/7] fixup: doc nit --- doc/api/stream.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/stream.md b/doc/api/stream.md index 9a2ba37c34f527..afc459dbeb08b5 100644 --- a/doc/api/stream.md +++ b/doc/api/stream.md @@ -1084,7 +1084,7 @@ buffer will be returned. If the `size` argument is not specified, all of the data contained in the internal buffer will be returned. -The `size` argument must be less than or equal to 2^30. +The `size` argument must be less than or equal to 1 GB. The `readable.read()` method should only be called on `Readable` streams operating in paused mode. In flowing mode, `readable.read()` is called From ba28187598ef87e9954358b994b3527d5fdbd7a8 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 20 Oct 2019 12:02:32 +0200 Subject: [PATCH 6/7] fixup: use primordials --- lib/_stream_readable.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index cd09be923c04dd..a365c802c36ad6 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -43,6 +43,7 @@ const { ERR_METHOD_NOT_IMPLEMENTED, ERR_STREAM_UNSHIFT_AFTER_END_EVENT } = require('internal/errors').codes; +const { Math } = primordials; // Lazy loaded to improve the startup performance. let StringDecoder; From a5b66425a96bbe80f56554c33901e1177c867923 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 20 Oct 2019 14:28:46 +0200 Subject: [PATCH 7/7] fixup: use hex --- lib/_stream_readable.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/_stream_readable.js b/lib/_stream_readable.js index a365c802c36ad6..99819f3b5fd5ca 100644 --- a/lib/_stream_readable.js +++ b/lib/_stream_readable.js @@ -43,7 +43,6 @@ const { ERR_METHOD_NOT_IMPLEMENTED, ERR_STREAM_UNSHIFT_AFTER_END_EVENT } = require('internal/errors').codes; -const { Math } = primordials; // Lazy loaded to improve the startup performance. let StringDecoder; @@ -376,7 +375,7 @@ Readable.prototype.setEncoding = function(enc) { }; // Don't raise the hwm > 1GB -const MAX_HWM = Math.pow(2, 30); +const MAX_HWM = 0x40000000; function computeNewHighWaterMark(n) { if (n >= MAX_HWM) { // TODO(ronag): Throw ERR_VALUE_OUT_OF_RANGE.