stream: check type and range of highWaterMark #13065

tniessen · 2017-05-16T22:36:07Z

Adds checks for the type of "highWaterMark" and restricts it to non-negative values as suggested here by @mscdex.

Refs: #12593

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

stream

Adds checks for the type of "highWaterMark" and restricts it to non-negative values. Refs: nodejs#12593

mscdex · 2017-05-16T23:37:06Z

lib/_stream_readable.js

-  this.highWaterMark = (hwm || hwm === 0) ? hwm : defaultHwm;
+  if (hwm == null) {
+    var defaultHwm = this.objectMode ? 16 : 16 * 1024;
+    this.highWaterMark = defaultHwm;


Can't we just move the ternary to here instead?

Sure, I wanted to, but I think I got an opposing comment when I did the same in one of the first commits of #12593. I cannot find it though, so I will go ahead and change it.

Edit: Done.

mscdex · 2017-05-16T23:37:34Z

lib/_stream_writable.js

-  this.highWaterMark = (hwm || hwm === 0) ? hwm : defaultHwm;
+  if (hwm == null) {
+    var defaultHwm = this.objectMode ? 16 : 16 * 1024;
+    this.highWaterMark = defaultHwm;


mcollina · 2017-05-19T08:41:31Z

I am 👎 in shipping this for v8.0.0, as it will require a major bump of readable-streamas well.
cc @calvinmetcalf what do you think?

mscdex · 2017-05-19T09:16:54Z

FWIW this PR is definitely supposed to be semver-major. As for which major version it gets shipped with, I don't particularly have any opinion on it.

tniessen · 2017-05-19T13:12:54Z

Deciding whether and when to include this patch is entirely up to you, I just thought it was consensus that it should be implemented at some point.

mcollina · 2017-05-19T13:15:30Z

@tniessen there is consensus that is needed :). I don't think we are ready to bump readable-stream major right now.

calvinmetcalf · 2017-05-22T12:03:50Z

I'm super against bumping readable-stream just on the grounds that nobody is going to upgrade anytime soon, we should at least put in a warning first and see how many people complain

jasnell · 2017-05-22T14:35:59Z

At this point it wouldn't make it in time for 8.0.0 anyway (which comes out in just over a week). I'm going to be pulling some semver-majors in either later today or tomorrow but I'm going to be very selective about it.

tniessen · 2017-06-01T14:59:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8402/

BridgeAR

Overall LGTM just a few nits.

BridgeAR · 2017-06-21T15:59:40Z

lib/_stream_readable.js

+      throw new RangeError('"highWaterMark" must not be negative');
+    }
+
+    // cast to ints.


Nit: I think it's better to start a comment with upper case. The same in the other file.

Yeah this is a remainder from the old code.

BridgeAR · 2017-06-21T16:02:39Z

lib/_stream_writable.js

+
+    // cast to ints.
+    this.highWaterMark = Math.floor(hwm);
+  }


I recommend to move this part into a generic function that can be called by readable and writable stream to prevent code duplication. This also keeps it always in line as the limits stay the same in both cases.

BridgeAR · 2017-06-21T16:06:04Z

lib/_stream_writable.js

-  // cast to ints.
-  this.highWaterMark = Math.floor(this.highWaterMark);
+    if (hwm < 0) {
+      throw new RangeError('"highWaterMark" must not be negative');


It would be nice if the internal error types would be used for all new errors.

This is not actually the case for streams. It's a way bigger chunk of work that we need to do, as we would have to support them in readable-stream as well.

I see, thanks for the update 👍

calvinmetcalf · 2017-06-21T16:27:57Z

so my current inclination is that this is not worth a major bump of readable-streams by itself

BridgeAR · 2017-08-27T00:06:19Z

@calvinmetcalf do you think this could land in 9.0.0?

mcollina · 2017-08-27T11:36:46Z

I am 👎 on landing this on 9.

jasnell · 2017-08-29T05:24:16Z

@mcollina ... is that a -1 to landing this at all or just right now?

mcollina · 2017-08-29T05:27:02Z

right now. I'm +1 on the change on itself.

BridgeAR · 2017-08-29T05:29:41Z

I added the blocked label. I guess as soon as streams get the next major there will be a couple PRs for it.

tniessen · 2017-08-29T08:44:58Z

I will do the rebase, address the nits etc. once the streams WG (namely @mcollina) decides when to ship this.

BridgeAR · 2017-11-22T11:49:58Z

@mcollina are you fine with landing this on v10? In that case it could be continued.

mcollina · 2017-12-08T18:38:03Z

I think we should, but I would like to keep thinking about it a bit more. Can we leave it as is?

BridgeAR · 2017-12-15T23:37:50Z

@mcollina as far as I know we land a couple of stream changes at the moment that might be breaking, so I personally feel like it is the right time to land this as well.

mcollina · 2018-01-08T09:15:07Z

I'm 👍 with this change in Node 10.

Anyone against this? cc @nodejs/streams

mcollina · 2018-01-08T09:15:53Z

Can you update the errors to use the new error codes? Those are supported in streams now.

tniessen · 2018-01-11T11:35:21Z

Closing this in favor of #18098.

tniessen added 2 commits May 17, 2017 00:15

stream: check highWaterMark type and range

b09ff75

Adds checks for the type of "highWaterMark" and restricts it to non-negative values. Refs: nodejs#12593

Add test cases

0545183

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label May 16, 2017

mscdex reviewed May 16, 2017

View reviewed changes

Remove unnecessary line

606eb59

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 19, 2017

mcollina mentioned this pull request May 22, 2017

bumping the major nodejs/readable-stream#284

Closed

jasnell added this to the 9.0.0 milestone May 22, 2017

BridgeAR requested changes Jun 21, 2017

View reviewed changes

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Aug 29, 2017

targos modified the milestones: 9.0.0, 10.0.0 Oct 23, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

tniessen mentioned this pull request Jan 11, 2018

stream: check type and range of highWaterMark #18098

Closed

4 tasks

tniessen closed this Jan 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: check type and range of highWaterMark #13065

stream: check type and range of highWaterMark #13065

tniessen commented May 16, 2017

mscdex May 16, 2017

tniessen May 17, 2017 •

edited

Loading

mscdex May 16, 2017

mcollina commented May 19, 2017

mscdex commented May 19, 2017

tniessen commented May 19, 2017

mcollina commented May 19, 2017

calvinmetcalf commented May 22, 2017

jasnell commented May 22, 2017

tniessen commented Jun 1, 2017

BridgeAR left a comment

BridgeAR Jun 21, 2017

tniessen Jun 21, 2017

BridgeAR Jun 21, 2017

BridgeAR Jun 21, 2017

mcollina Jun 21, 2017

BridgeAR Jun 21, 2017

calvinmetcalf commented Jun 21, 2017

BridgeAR commented Aug 27, 2017

mcollina commented Aug 27, 2017

jasnell commented Aug 29, 2017

mcollina commented Aug 29, 2017

BridgeAR commented Aug 29, 2017

tniessen commented Aug 29, 2017

BridgeAR commented Nov 22, 2017

mcollina commented Dec 8, 2017

BridgeAR commented Dec 15, 2017

mcollina commented Jan 8, 2018

mcollina commented Jan 8, 2018

tniessen commented Jan 11, 2018

stream: check type and range of highWaterMark #13065

stream: check type and range of highWaterMark #13065

Conversation

tniessen commented May 16, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

tniessen May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented May 19, 2017

mscdex commented May 19, 2017

tniessen commented May 19, 2017

mcollina commented May 19, 2017

calvinmetcalf commented May 22, 2017

jasnell commented May 22, 2017

tniessen commented Jun 1, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinmetcalf commented Jun 21, 2017

BridgeAR commented Aug 27, 2017

mcollina commented Aug 27, 2017

jasnell commented Aug 29, 2017

mcollina commented Aug 29, 2017

BridgeAR commented Aug 29, 2017

tniessen commented Aug 29, 2017

BridgeAR commented Nov 22, 2017

mcollina commented Dec 8, 2017

BridgeAR commented Dec 15, 2017

mcollina commented Jan 8, 2018

mcollina commented Jan 8, 2018

tniessen commented Jan 11, 2018

tniessen May 17, 2017 •

edited

Loading