in transform stream transform method, `callback(null, null)` doesn't end stream but `this.push(null); callback()` does

[ShenHongFei]

Version

latest main branch

Platform

irrelevant

Subsystem

stream

What steps will reproduce the bug?

https://nodejs.org/api/stream.html#transform-transformchunk-encoding-callback

callback(null, null) doesn't end stream but this.push(null); callback() does

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

Always

What is the expected behavior? Why is that the expected behavior?

callback(null, null) and this.push(null); callback() have the same effect (the stream ends).

What do you see instead?

callback(null, null) is different from this.push(null); callback()

Additional information

https://github.com/nodejs/node/blob/main/lib/internal/streams/transform.js#L177-L179

Would it be possible to change this to the following?

if (val !== undefined) {
  this.push(val)
}

[trivikr]

Should callback(null, null) end stream? It's debatable.

In a Transform, callback(error, data) treats the second argument as output data only when it is not null/undefined. So callback(null, null) means “the transform for this chunk completed and produced no output”, not “push EOF”. Ending the readable side is normally done by push(null), by the transform finishing normally after writable end, or by _flush/finalization behavior.

Do docs imply null callback data is equivalent to push(null)?

[ShenHongFei]

@trivikr Yes, that's what the document says.

transform._transform(chunk, encoding, callback)

...

• callback {Function} A callback function (optionally with an error
  argument and data) to be called after the supplied chunk has been
  processed.

This function MUST NOT be called by application code directly. It should be
implemented by child classes, and called by the internal Readable class
methods only.

All Transform stream implementations must provide a _transform()
method to accept input and produce output. The transform._transform()
implementation handles the bytes being written, computes an output, then passes
that output off to the readable portion using the transform.push() method.

The transform.push() method may be called zero or more times to generate
output from a single input chunk, depending on how much is to be output
as a result of the chunk.

It is possible that no output is generated from any given chunk of input data.

The callback function must be called only when the current chunk is completely
consumed. The first argument passed to the callback must be an Error object
if an error occurred while processing the input or null otherwise. If a second
argument is passed to the callback, it will be forwarded on to the
transform.push() method, but only if the first argument is falsy. In other
words, the following are equivalent:

transform.prototype._transform = function(data, encoding, callback) {
  this.push(data);
  callback();
};

transform.prototype._transform = function(data, encoding, callback) {
  callback(null, data);
};

...

[trivikr]

From the docs, the second argument “will be forwarded on to the transform.push() method” when the error is falsy. Since readable.push(null) means EOF, that wording implies callback(null, null) could mean EOF too.

Since the current behavior is intentional or at least established, this should mostly be a doc fix.

[ShenHongFei]

node/lib/internal/streams/transform.js

Lines 138 to 140 in 614050b

	if (data != null) {
	this.push(data);
	}

I checked the document and found that only the callback functions of the two APIs, transform._flush(callback) and transform._transform(chunk, encoding, callback), accept (error?) , data?). For the two parameters, it needs to be clarified in the documentation that the push(data) function will only be called if the 'data' parameter is not null or undefined.

[ShenHongFei]

#62803
@trivikr Here is a pr made by @galaxy4276 to the implementation rather than the documentation. Could you please advise on the best course of action?

[trivikr]

Thanks for the research @ShenHongFei

I've requested existing reviewers to re-approve and land the fix in #62803 (comment)

[github-actions]

This issue has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

[github-actions]

This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.