Node 16.14.2 highWaterMark:0 doesn't handle backpressure

[paulrutter]

Version

16.14.2

Platform

x64

Subsystem

No response

What steps will reproduce the bug?

async function start() {
	const { Transform } = require('stream');
	const transformStream = new Transform({
		objectMode: true,
		highWaterMark: 0,

		transform(item, enc, callback) {
	  	  console.error("writing", item);
	  	  this.push(item);
	  	  callback();
	    }
	});

	transformStream.write('hello1');
	transformStream.write('hello2');
	transformStream.write('hello3');
	transformStream.write('hello4');
	transformStream.write('hello5');
	transformStream.write('hello6');
	transformStream.write('hello7');
	transformStream.write('hello8');
	transformStream.write('hello9');
        transformStream.end();

	for await (const text of transformStream) {
	  await wait();
	  console.error(text);
	}
};

async function wait() {
  return new Promise(resolve => {
	setTimeout(() => resolve(), 1000);
  });
}

start();

I wouldn't expect the transform method to be called, while the highWaterMark is already reached.

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

Node 14 and older, backpressure is handled properly (even with highWaterMark: 0):

Expected output:

writing hello1
writing hello2
hello1
hello2
writing hello3
hello3
writing hello4
hello4
writing hello5
hello5
writing hello6
hello6
writing hello7
hello7
writing hello8
hello8
writing hello9
hello9

What do you see instead?

PS > node .\backpressure.js
writing hello1
writing hello2
writing hello3
writing hello4
writing hello5
writing hello6
writing hello7
writing hello8
writing hello9
hello1
hello2
hello3
hello4
hello5
hello6
hello7
hello8
hello9

Additional information

When using highWaterMark: 1 in Node 16.x, backpressure works as expected again.
Did anything change around using highWaterMark:0 in Node16? It doesn't seem backwards compatible.

[paulrutter]

Probably related changes: #40935 and #40947

[kanongil]

Indeed, just noticed this issue with a test case that is now failing on node 16.14.0+. Does anyone care about this regression? @ronag?

[targos]

@nodejs/streams

[mcollina]

It seems #40947 should not have been backported at least.

@kanongil @paulrutter what behavior were you expecting for highWaterMark: 0? Was this documented anywhere at all?

[paulrutter]

My thought with using highwatermark 0 was to not buffer any chunks, instead just wait until the current chunk is done and then proceed to the next chunk.
I don't think the docs describe using 0 specifically, but it worked for my usecase so i didn’t give it more thought until it broke in node16.

[mcollina]

Essentially you assumed that it would be the same behavior of highWaterMark: 1?

In my mind a value of 0 would be "false", so no buffering at all. The current behavior matches that description.

[paulrutter]

Yes, true. In hindsight, using 1 would have made more sense. Still i think it's not great that the change is not backwards compatible anymore.

[mcollina]

In that I agree.

@nodejs/tsc @nodejs/releasers, what should we do?

[kanongil]

I use highWaterMark: 0 to enable an object mode transform stream to be pull-based.

This means a call to stream.read() will initially return null, and once 'readable' triggers, I can stream.read() a single element out of it. Then I have the option to do another stream.read() to trigger a new pull, or just wait until it suits me.

When applied to a Transform stream that is piped into, it means that it internally only stores 1 incoming object (in the write end), before applying backpressure.

writableHighWaterMark: 0, readableHighWaterMark: 1 doesn't work, since it will cause 2 objects to be internally buffered. 1 on the write side, and 1 in the read side.

[mcollina]

@ronag you should chime in on this one, because it looks like we might want to revert to the original behavior.

[ronag]

I'll try to have a look next week.