-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
stream: support readable/writableHighWaterMark for Duplex streams #14636
Conversation
cc/ @nodejs/streams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! There are only some nits to address!
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, | ||
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR | ||
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE | ||
// USE OR OTHER DEALINGS IN THE SOFTWARE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the copyright header is not needed for new files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
const DEFAULT = 16 * 1024; | ||
|
||
function test_transform(expected_readable_hwm, expected_writable_hwm, options) { | ||
console.log(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this console.log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const Writable = stream.Writable; | ||
const DEFAULT = 16 * 1024; | ||
|
||
function test_transform(expected_readable_hwm, expected_writable_hwm, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use camelCase in general. You can use shorter names if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
771f20e
to
078b4ee
Compare
@mcollina Thanks for the review! I fixed the comments and squashed. |
@lpinca I'm trying to understand if the failure is related to my change, and I suspect it doesn't. |
@guymguym you are correct, it's not related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few comments.
const assert = require('assert'); | ||
const stream = require('stream'); | ||
|
||
const Transform = stream.Transform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you grab these 3 things using destructuring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
testTransform(DEFAULT, DEFAULT, { writableHighWaterMark: NaN }); | ||
|
||
// test highWaterMark = undefined, null, NaN | ||
testTransform(666, DEFAULT, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could reduce duplication by doing something like this throughout the test:
[undefined, null, NaN].forEach((highWaterMark) => {
testTransform(666, DEFAULT, {
highWaterMark, readableHighWaterMark: 666
});
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
078b4ee
to
13a8f7b
Compare
@cjihrig Thanks, fixed and squashed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed as c3c045a. |
Congratulations for your first contribution to Node.js! |
I've added the semver-minor flag as it's a new feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments a bit late. If we get some agreement I'll open a PR.
@@ -1752,6 +1752,10 @@ constructor and implement *both* the `readable._read()` and | |||
* `writableObjectMode` {boolean} Defaults to `false`. Sets `objectMode` | |||
for writable side of the stream. Has no effect if `objectMode` | |||
is `true`. | |||
* `readableHighWaterMark` {number} Sets `highWaterMark` for the readable side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions:
Add a link to [#buffering]
Also in the first *
(options
) add links to [#constructor-new-streamwritableoptions] and [#new-streamreadableoptions]
@@ -1752,6 +1752,10 @@ constructor and implement *both* the `readable._read()` and | |||
* `writableObjectMode` {boolean} Defaults to `false`. Sets `objectMode` | |||
for writable side of the stream. Has no effect if `objectMode` | |||
is `true`. | |||
* `readableHighWaterMark` {number} Sets `highWaterMark` for the readable side | |||
of the stream. Has no effect if `highWaterMark` is provided. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd phrase it as The general `highWaterMark` option overrides this one.
IMHO a positive-active sentence is a little bit clearer than a negative-passive one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that positive is better. I just copied this format from readable/writableObjectMode.
// However, some cases require setting options to different | ||
// values for the readable and the writable sides of the duplex stream. | ||
// These options can be provided separately as readableXXX and writableXXX. | ||
var isDuplex = stream instanceof Stream.Duplex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina isn't instanceof
discouraged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but this was already there, this PR just moves it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion it is best to allow it also for Readable/Writable without checking for Duplex, because this is an inherited behavior that behaves different on the base class vs the child class, and this is more confusing than helping. Not sure if this will be agreed by any developer reading the docs though. I wonder what you guys think.
}); | ||
|
||
// test undefined, null, NaN | ||
[undefined, null, NaN].forEach((v) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens with ['', 'foo', ()=>{}, Symbol('frog'), {}, []]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@refack The behavior will be unspecified in these cases, since highWaterMark is expected to be a number, but only checked to be a truthy value or 0.
This was also the case before my PR, though I would prefer it to be a specified behavior instead.
Perhaps the behavior should be to ignore non number values or else throw TypeError as a breaking change...
@refack go ahead and fire a PR. |
Notable changes * **Inspector** * `require()` is available in the inspector console now. [#8837](#8837) * **N-API** * New APIs for creating number values have been introduced. [#14573](#14573) * **Stream** * For `Duplex` streams, the high water mark option can now be set independently for the readable and the writable side. [#14636](#14636) * **Util** * `util.format` now supports the `%o` and `%O` specifiers for printing objects. [#14558](#14558) PR-URL: #14811
Notable changes * **HTTP2** * Experimental support for the built-in `http2` has been added via the `--expose-http2` flag. [#14239](#14239) * **Inspector** * `require()` is available in the inspector console now. [#8837](#8837) * Multiple contexts, as created by the `vm` module, are supported now. [#14465](#14465) * **N-API** * New APIs for creating number values have been introduced. [#14573](#14573) * **Stream** * For `Duplex` streams, the high water mark option can now be set independently for the readable and the writable side. [#14636](#14636) * **Util** * `util.format` now supports the `%o` and `%O` specifiers for printing objects. [#14558](#14558) PR-URL: #14811
Notable changes * **HTTP2** * Experimental support for the built-in `http2` has been added via the `--expose-http2` flag. [#14239](#14239) * **Inspector** * `require()` is available in the inspector console now. [#8837](#8837) * Multiple contexts, as created by the `vm` module, are supported now. [#14465](#14465) * **N-API** * New APIs for creating number values have been introduced. [#14573](#14573) * **Stream** * For `Duplex` streams, the high water mark option can now be set independently for the readable and the writable side. [#14636](#14636) * **Util** * `util.format` now supports the `%o` and `%O` specifiers for printing objects. [#14558](#14558) PR-URL: #14811
Notable changes * **HTTP2** * Experimental support for the built-in `http2` has been added via the `--expose-http2` flag. [#14239](nodejs/node#14239) * **Inspector** * `require()` is available in the inspector console now. [#8837](nodejs/node#8837) * Multiple contexts, as created by the `vm` module, are supported now. [#14465](nodejs/node#14465) * **N-API** * New APIs for creating number values have been introduced. [#14573](nodejs/node#14573) * **Stream** * For `Duplex` streams, the high water mark option can now be set independently for the readable and the writable side. [#14636](nodejs/node#14636) * **Util** * `util.format` now supports the `%o` and `%O` specifiers for printing objects. [#14558](nodejs/node#14558) PR-URL: nodejs/node#14811
Fixes: #14555
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
stream