stream: check type and range of highWaterMark

[tniessen]

Continuation of #13065.

Stream constructors should throw when an invalid highWaterMark option is given.

Checklist

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

Affected core subsystem(s)

stream

[tniessen]

There are two aspects which I would like to discuss:

1. NaN is accepted but ignored. This is the expected behavior according to test-stream-transform-split-highwatermark, but I don't see much value in that.
2. Floating point numbers are permitted and rounded to integers.

[mcollina]

I'm 👍 in removing support for NaN, i.e. throwing if it is passed.
I would keep the rounding, as it might happen that highWaterMark  is the result of some division, and I don't think the rounding harms

[BridgeAR]

I suggest to implement this in a more efficient way.

I also agree with @mcollina that NaN should not be supported anymore.

LGTM otherwise!

[BridgeAR]

The following should be more efficient (untested):

function getHighWaterMark (options, isDuplex, specialKey, objectMode) {
  if (typeof options.highWaterMark === 'number') {
    if (options.highWaterMark < 0 || Number.isNaN(options.highWaterMark)) {
      throw new errors.TypeError(
        'ERR_INVALID_OPT_VALUE',
        'options.highWaterMark',
        options.highWaterMark
      )
    }
    return Math.floor(options.highWaterMark)
  }
  if (isDuplex) {
    if (typeof options[specialKey] === 'number') {
      if (options[specialKey] < 0 || Number.isNaN(options[specialKey])) {
        throw new TypeError(...)
      }
      return Math.floor(options[specialKey])
    }
    if (options[specialKey]) {
      throw new TypeError(...)
    }
  } else if(options.highWaterMark) {
    throw new TypeError(...)
  }

  return objectMode ? 16 : 16 * 1024
}

getHighWaterMark(
  options,
  isDuplex,
  'writableHighWaterMark', // Replace accordingly
  this.objectMode
)

[tniessen]

Yeah I was going to extract the key anyway, just wanted to hear opinions about NaN first (that really annoyed me).

[tniessen]

By the way, !(options.highWaterMark >= 0) should be faster than options.highWaterMark < 0 || Number.isNaN(options.highWaterMark).

[BridgeAR]

That is probably true.

[mcollina]

LGTM

[mcollina]

This needs a CITGM before landing run after #18210 lands and we get CITGM back.

[tniessen]

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1218/

[tniessen]

citgm looks good to me, can someone confirm?

[addaleax]

@tniessen yup, looks good to me too 👍

[tniessen]

CI: https://ci.nodejs.org/job/node-test-commit/15588/
CI: https://ci.nodejs.org/job/node-test-pull-request/12661/

[tniessen]

Landed in 46e0a55.