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.

[ronag]

I'm not sure hwm of 0 makes any sense. Maybe the simple solution is to simply automatically change 0 to 1?

[kanongil]

I'm not sure hwm of 0 makes any sense. Maybe the simple solution is to simply automatically change 0 to 1?

I have been using it, and I will have to rework my Transform stream to Duplex stream if it is limited to 1, or drop node 16+ support. See #42457 (comment).

[richardlau]

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.

In that I agree.

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

The Release WG discussed this in today's meeting. The next regular 16 release is planned for sometime in July (after next week's security releases). It would be good to get some guidance from @nodejs/streams before then regarding whether to revert #40947, leave it as-is, or whether some other patch should land on v16.x.

[kanongil]

I did some digging, and found a potential fix that solves both regressions.

The fix in #40947 is indeed faulty. The real issue is that _read() is not called when doing a single stream.read() call, since data is already in the buffer from a stream.push() and highWaterMark is 0.

The real fix seems to be to force the readable side of the transform stream to always call _read() when deferring the callback to it here:

node/lib/internal/streams/transform.js

Line 173 in 736a7d8

this[kCallback] = callback;

This can be forced with a rState.needReadable = true;, since it uses that as the primary signal to call _read() on a read() call.

Note I haven't tested this fix against the full node.js test suite, but it works for both my code and the testcase from #40947.

[mcollina]

Would you like to send a PR?

[kanongil]

@mcollina Yes, looking into making a PR right now.

[kanongil]

Hmm, it appears that my issue it not the same as this (at least the reproduction code). The reproduction code relies on calling write() into a stream that returns false. This makes the case fundamentally incompatible with the new transform-on-write model which was already present in v16.0.

As such my fix doesn't handle this exact issue, though it might fix the actual issue this is based on.

[kanongil]

PR up in #43648. As per my previous comment, it doesn't fix the exact reproduction code from this issue, but it fixes the regression in my transform stream.