stream: increase MAX_HWM by ronag · Pull Request #29938 · nodejs/node · GitHub

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: #29933

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

@addaleax @mcollina

Can you add some description/comments to the test? It would be more explicit to add some assertions as well.

Added simple description. Not sure what assertion would be relevant here?

Can you also add a test for file streams specifically, along the lines of the reproduction in the original issue?

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

@nodejs/streams

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

Test failures look like this:

Mismatched noop function calls. Expected exactly 1, actual 0.
    at Object.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:337:10)
    at Object.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-stream-readable-read-max-hwm.js:29:20)
    at Module._compile (internal/modules/cjs/loader.js:958:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:994:10)
    at Module.load (internal/modules/cjs/loader.js:813:32)
    at Function.Module._load (internal/modules/cjs/loader.js:725:14)
    at Function.Module.runMain (internal/modules/cjs/loader.js:1046:10)
    at internal/main/run_main_module.js:17:11

@Trott: Picking another bufferSize triggered another type of failure. I've partly fixed it but not entirely.

So though this fixes the referred issue it doesn't fix every issue. I will need to spend more time on this.

@mcollina @addaleax I think fixing this is basically going to end up with almost identical behaviour as not having a MAX_HWM at all... I'm not sure what the rationale for MAX_HWM was.

I need a little guidance on what we would prefer here:

Leave as is.
Partly fix (current state of this PR)
Make and document size purely as a hint (which is what it kind of is now due to MAX_HWM).
Add an error if read(size) exceeds MAX_HWM and try to increase the MAX_HWM value to a value high enough that it doesn't break most of existing usage.
Temporarily increase highWaterMark to requested size and then bring it back down again once length has reached the required length to fulfil one read of that size.

I would recommend a scavenge in history in why there is a MAX_HWM at all.
It was added in 9208c89 to avoid pathological failures with synchronous calls to _read after push(). Those are not there anymore since Node 10, so MAX_HWM can be removed.

It would be good to do a git bisect to find out which commit broke this (likely is one of mine), so we would know more.

@mcollina: Thanks. Didn't even consider removing it. Updated PR.

@Trott: This is no longer WIP

@Trott: I think this is ready?

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

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

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

@Trott ping

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

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

Landed in f8c069f.

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: nodejs#29933 PR-URL: nodejs#29938 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@BridgeAR this should probably be backported to node 12?

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: #29933 PR-URL: #29938 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Oct 11, 2019

ronag mentioned this pull request Oct 11, 2019

attempting to use a readstream to read a specific size of bytes no longer works properly with Node 10 #29933

Closed

ronag force-pushed the fix-readable-max-hwm branch 3 times, most recently from 595a301 to 21015cd Compare October 11, 2019 20:25

mcollina reviewed Oct 12, 2019

View reviewed changes

ronag force-pushed the fix-readable-max-hwm branch from 1d5d383 to 310821f Compare October 12, 2019 11:56

addaleax approved these changes Oct 12, 2019

View reviewed changes

Fishrock123 reviewed Oct 12, 2019

View reviewed changes

Comment thread test/parallel/test-stream-readable-read-max-hwm.js Outdated

ronag force-pushed the fix-readable-max-hwm branch 2 times, most recently from bc62925 to 0d228cb Compare October 12, 2019 21:21

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 13, 2019

Trott reviewed Oct 13, 2019

View reviewed changes

Comment thread test/parallel/test-stream-readable-read-max-hwm.js Outdated

Trott removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 14, 2019

Trott added the wip Issues and PRs that are still a work in progress. label Oct 15, 2019

ronag force-pushed the fix-readable-max-hwm branch from 03a686d to d147179 Compare October 20, 2019 08:20

ronag force-pushed the fix-readable-max-hwm branch 3 times, most recently from b8288e5 to 3a63e31 Compare October 20, 2019 08:25

ronag requested a review from Fishrock123 October 20, 2019 08:26

fixup: use primordials

ba28187

ronag force-pushed the fix-readable-max-hwm branch from 4f0a6dd to ba28187 Compare October 20, 2019 10:48

fixup: use hex

a5b6642

Trott removed the wip Issues and PRs that are still a work in progress. label Oct 21, 2019

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 3, 2019

Trott closed this Nov 12, 2019

MylesBorins added lts-watch-v10.x labels Nov 17, 2019

BridgeAR mentioned this pull request Nov 19, 2019

v13.2.0 proposal #30547

Merged

targos removed the lts-watch-v12.x label Dec 1, 2019

BethGriggs mentioned this pull request Dec 9, 2019

v12.13.2 proposal #30865

Closed

BethGriggs mentioned this pull request Dec 23, 2019

v12.14.1 proposal #31069

Merged

BethGriggs removed the lts-watch-v10.x label Jan 7, 2020

BethGriggs mentioned this pull request Jan 7, 2020

v10.18.1 proposal #31248

Merged

stream: increase MAX_HWM by ronag · Pull Request #29938 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: increase MAX_HWM - #29938

stream: increase MAX_HWM#29938ronag wants to merge 7 commits intonodejs:masternodejs/node:masterfrom nxtedition:fix-readable-max-hwmnxtedition/node:fix-readable-max-hwmCopy head branch name to clipboard

Conversation

ronag commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Oct 11, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Oct 12, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 13, 2019

Uh oh!

Trott commented Oct 13, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 14, 2019

Uh oh!

Trott commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Oct 15, 2019

Uh oh!

ronag commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Oct 19, 2019

Uh oh!

ronag commented Oct 20, 2019

Uh oh!

ronag commented Oct 20, 2019

Uh oh!

ronag commented Nov 1, 2019

Uh oh!

nodejs-github-bot commented Nov 3, 2019

Uh oh!

nodejs-github-bot commented Nov 5, 2019

Uh oh!

nodejs-github-bot commented Nov 6, 2019

Uh oh!

ronag commented Nov 11, 2019

Uh oh!

nodejs-github-bot commented Nov 11, 2019

Uh oh!

nodejs-github-bot commented Nov 12, 2019

Uh oh!

Trott commented Nov 12, 2019

Uh oh!

ronag commented Nov 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

stream: increase MAX_HWM#29938
ronag wants to merge 7 commits into
nodejs:masterfrom
nxtedition:fix-readable-max-hwm

ronag commented Oct 11, 2019 •
edited
Loading

Trott commented Oct 14, 2019 •
edited
Loading

ronag commented Oct 15, 2019 •
edited
Loading