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: check type and range of highWaterMark #18098
Conversation
The (h|readableH|writableH)ighWaterMark options should only permit positive numbers and zero.
|
There are two aspects which I would like to discuss:
|
|
I'm |
|
|
||
| // Default value | ||
| return state.objectMode ? 16 : 16 * 1024; | ||
| } |
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 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
)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.
Yeah I was going to extract the key anyway, just wanted to hear opinions about NaN first (that really annoyed me).
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.
By the way, !(options.highWaterMark >= 0) should be faster than options.highWaterMark < 0 || Number.isNaN(options.highWaterMark).
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.
That is probably true.
|
This needs a CITGM before landing run after #18210 lands and we get CITGM back. |
|
citgm looks good to me, can someone confirm? |
|
@tniessen yup, looks good to me too |
The (h|readableH|writableH)ighWaterMark options should only permit positive numbers and zero. PR-URL: #18098 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Landed in 46e0a55. |
The (h|readableH|writableH)ighWaterMark options should only permit positive numbers and zero. PR-URL: nodejs#18098 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Continuation of #13065.
Stream constructors should throw when an invalid
highWaterMarkoption is given.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
stream