stream: fix 0 transform hwm backpressure

[ronag]

Refs: #42457
Refs: https://github.com/nodejs/node/pull/43648/files

This also fixes a long standing issue where setting a hwm on a transform stream actually causes twice as much buffering as expected.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[ronag]

@kanongil

[mcollina]

lgtm

[kanongil]

This patch does not fix my issue since it removes the ability to defer the call to _read() / _transform() by waiting to call read().

It seems you have modified the test code I provided here to suit your patch, by performing an immediate read() after the first read(). My original test code (that works on node v14 and node v16.3) will fail with this patch!

[kanongil]

Incidentally, the actual test that was failing in my code, will work with this patch, since I explicitly pass readableHighWaterMark: 0. Though the resulting stream would fail your drain test, since it doesn't pre-fetch.

[ronag]

This patch does not fix my issue since it removes the ability to defer the call to _read() / _transform() by waiting to call read().

It seems you have modified the test code I provided here to suit your patch, by performing an immediate read() after the first read(). My original test code (that works on node v14 and node v16.3) will fail with this patch!

I'm not sure I understand. Node streams are greedy so I don't think deferring a read is actually part of the design. Also this is a separate issue than what I'm trying to solve, which is the lack of back pressure.

Making a lazy transform by passing 0 is IMHO a separate issue and something I need to further consider.

[kanongil]

The lazy reads used to work. Whether it is by design or not.

Anyway, you are correct that this fixes the backpressure issue for highWaterMark: 0, and at most 1 element should be buffered using this patch. It does not fix it for writableHighWaterMark: 0, readableHighWaterMark: 0.

[kanongil]

Just because you haven't thought that transfrom streams can be lazy read, it has worked so far, and makes sense given how it is documented. It is also used internally for process.stdin TTY afaik, where you don't want to read user input greedily.

At the very least, this pre-load behaviour makes it inconsistent with how a standard Readable works. Eg. for

const r = new Readable({
  highWaterMark: 0,
  read() {
    console.log('_read');
    process.nextTick(() => this.push('a'))
  }
});

If you do a single r.read() between each nextTick it will alternate between returning the pushed value, and scheduling a new _read().

Also, removing the feature mid-release cycle (node v16.3 => v.16.4) seems like an error.

[benjamingr]

Apologies to the ecosystem for changing everyone's buffering in code using Node in advance.

[kanongil]

Apologies to the ecosystem for changing everyone's buffering in code using Node in advance.

I guess we were warned:

node/doc/api/stream.md

Lines 123 to 124 in b993789

	The mechanics of the internal buffering are an internal implementation detail
	and may be changed at any time. However, for certain advanced implementations,

[benjamingr]

@kanongil lucky for us users always read the docs, only use public APIs and always use them in ways they expect :D

[ronag]

Just to clarify. I'm not saying we shouldn't fix the lazy loading. But this PR is not intended to fix that and we can try to fix that in a separate PR. This is ONLY trying to fix the back pressure issue.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mcollina]

lgtm

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/43685
✔  Done loading data for nodejs/node/pull/43685
----------------------------------- PR info ------------------------------------
Title      stream: fix 0 transform hwm backpressure (#43685)
Author     Robert Nagy  (@ronag)
Branch     ronag:fix-42457 -> nodejs:main
Labels     stream, author ready, commit-queue-squash
Commits    16
 - stream: fix 0 transform hwm backpressure
 - stream: add 0 hwm test
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: fix 0 transform hwm backpressure
 - fixup! stream: add 0 hwm test
 - fixup
 - fixup: allow overriding writableHighWaterMark
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/43685
Refs: https://github.com/nodejs/node/issues/42457
Refs: https://github.com/nodejs/node/pull/43648/files
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43685
Refs: https://github.com/nodejs/node/issues/42457
Refs: https://github.com/nodejs/node/pull/43648/files
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 05 Jul 2022 08:58:47 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/43685#pullrequestreview-1041623843
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/43685#pullrequestreview-1028862249
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2022-07-17T15:50:44Z: https://ci.nodejs.org/job/node-test-pull-request/45494/
- Querying data for job/node-test-pull-request/45494/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2689876942

[mcollina]

git node land is failing but I do not understand why. What should I do in this case?

[aduh95]

git node land is failing but I do not understand why. What should I do in this case?

Land manually.

[aduh95]

Landed in 31d9edc