Skip to content
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: increase MAX_HWM #29938

Closed
wants to merge 7 commits into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Oct 11, 2019

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Oct 11, 2019
@ronag ronag force-pushed the fix-readable-max-hwm branch 3 times, most recently from 595a301 to 21015cd Compare October 11, 2019 20:25
@ronag
Copy link
Member Author

ronag commented Oct 11, 2019

@addaleax @mcollina

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ronag
Copy link
Member Author

ronag commented Oct 12, 2019

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ronag ronag force-pushed the fix-readable-max-hwm branch 2 times, most recently from bc62925 to 0d228cb Compare October 12, 2019 21:21
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Oct 13, 2019

@nodejs/streams

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 14, 2019
@Trott
Copy link
Member

Trott commented Oct 14, 2019

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

@ronag
Copy link
Member Author

ronag commented Oct 15, 2019

@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.

@ronag
Copy link
Member Author

ronag commented Oct 15, 2019

@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.

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

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.

@ronag
Copy link
Member Author

ronag commented Oct 20, 2019

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

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

ronag commented Oct 20, 2019

@Trott: This is no longer WIP

@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Oct 21, 2019
@ronag
Copy link
Member Author

ronag commented Nov 1, 2019

@Trott: I think this is ready?

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 3, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Nov 11, 2019

@Trott ping

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Nov 12, 2019

Landed in f8c069f.

@Trott Trott closed this Nov 12, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 12, 2019
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>
@ronag
Copy link
Member Author

ronag commented Nov 13, 2019

@BridgeAR this should probably be backported to node 12?

MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
targos pushed a commit that referenced this pull request Dec 1, 2019
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>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
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>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
BethGriggs pushed a commit that referenced this pull request Jan 7, 2020
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>
@BethGriggs BethGriggs mentioned this pull request Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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