stream: allow null as second arg in Transform callback

[galaxy4276]

What

The docs say passing a value as the second argument to the transform callback is equivalent to calling transform.push(value). That implies callback(null, null) should push null — ending the readable side — just like this.push(null); callback() does. It doesn't.

Bug

const { Transform } = require('stream');

const t = new Transform({
  transform(chunk, encoding, callback) {
    callback(null, null); // intend to signal EOF
  },
});

t.on('end', () => console.log('ended')); // never fires
t.write('hello');
t.end();

The 'end' event never fires. The stream hangs.

Root cause — lib/internal/streams/transform.js:

// Before
if (val != null) {   // loose equality: null == null → true → blocks push(null)
  this.push(val);
}

val != null uses loose equality, so both null and undefined fail the check. this.push(null) is never called, state.ended stays false, and 'end' is never emitted.

Fix

// After
if (val !== undefined) {  // only block undefined; null passes through
  this.push(val);
}

Now callback(null, null) reaches this.push(null), which sets state.ended = true and eventually emits 'end' once the buffer drains — matching what the docs describe.

Behavior after fix

const t = new Transform({
  transform(chunk, encoding, callback) {
    callback(null, null); // equivalent to: this.push(null); callback()
  },
});

t.on('end', () => console.log('ended')); // fires correctly now
t.write('hello');
t.end();

Potential concern

This is technically a behavior change. Code that relied on callback(null, null) not ending the stream should use callback() (no second argument) instead. I think this is the right call given the docs, but open to pushback — especially on whether this warrants a semver-minor label or a deprecation note before changing.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[galaxy4276]

Looking at the history, val != null has been in transform.js since the file moved to internal/streams (~2020), so this isn't a regression — it's been silently broken for a long time. The loose equality check was likely written to block undefined (when no second arg is passed), but it incidentally also blocks null, which is the EOF sentinel.

The fix changes to val !== undefined, which only blocks undefined and lets null through to this.push(null) — matching what the docs describe:

"If a second argument is passed to the callback, it will be forwarded on to the transform.push() method"

In practice, callback(null, null) to signal EOF is uncommon — most users write this.push(null); callback() — so the blast radius of this change should be small. There's also no legitimate use case for callback(null, null) meaning "push nothing and continue", since null is the EOF sentinel in both buffer and object mode.

That said, I'm uncertain whether this is semver-patch (bug fix aligning with docs) or semver-minor (observable behavior change). Would appreciate guidance on the right label and whether a CHANGELOG note is needed.

[nodejs-github-bot]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.69%. Comparing base (31b9e60) to head (4aefb07).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62803      +/-   ##
==========================================
- Coverage   89.70%   89.69%   -0.01%     
==========================================
  Files         706      706              
  Lines      218288   218222      -66     
  Branches    41782    41767      -15     
==========================================
- Hits       195806   195742      -64     
+ Misses      14394    14381      -13     
- Partials     8088     8099      +11     

Files with missing lines	Coverage Δ
lib/internal/streams/transform.js	98.52% <100.00%> (ø)

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[galaxy4276]

@lpinca @ronag The only change since your review is fixing a capitalized-comments lint error on line 21.
Would you mind re-approving?

[mcollina]

This should have a pass of citgm before landing.

I expect a significant breakage in the ecosystem.

[galaxy4276]

This should have a pass of citgm before landing.

I expect a significant breakage in the ecosystem.

Thank you for your review.
As expected, since I'm changing the internal implementation, testing will be necessary.
I'll conduct stability testing through citgm and attach the results.

[galaxy4276]

citgm run — results

TL;DR: Full citgm-all + extended stream subset + pattern audit show zero ecosystem regressions.
One pre-existing failure (JSONStream) is fixed by this change.
A semantic differential test confirms the behavior change is real but is not used by any library surveyed.

---

Environment

Field	Value
Platform	macOS Tahoe 26.4.1 (arm64)
Toolchain	Apple clang 21.0.0 / Xcode 26.4.1
Baseline	58a8e1da5d (main)
PR HEAD	4aefb0790b
citgm	v10.0.1, -j 4

Full citgm-all (72 modules)

	Baseline	PR
Pass	52	52
Fail (all pre-existing / env)	20	20
Regressions	—	0

Pre-existing failures (identical on both binaries)

@babel/core · @clinic/doctor · @yarnpkg/cli · ava · bufferutil · cheerio · commander · fastify · jose · jquery · koa · lru-cache · mime · nan · node-gyp · path-to-regexp · pino · prom-client · rewire · undici

All fail with the same error on both binaries (yarn resolution, native addon build on Apple clang 21, environment/flake). Spot-checked @babel/core: identical YN0060/YN0002 yarn errors baseline vs PR.

Extended stream subset (17 modules not in citgm lookup)

Ran against highland, duplexify, mississippi, parallel-transform, stream-to-promise, stream-each, stream-shift, get-stream, byline, JSONStream, ndjson, through, event-stream, from2, pump-file-to-s3, duplex-stream, multipipe.

	Baseline	PR
Pass	9	10
Fail (pre-existing)	8	7
Regressions	—	0
Fixes	—	1 (JSONStream)

Pattern audit

Grepped (callback\|cb\|done\|next)\s*\(\s*null\s*,\s*null across:

• Top 50 most-downloaded npm packages
• 24 stream-centric libraries (readable-stream, through2, pump, pumpify, duplexify, split2, parallel-transform, …)

Hits outside Transform context: webpack (7), undici (2), mongoose (8) — all in unrelated async callbacks (resolver cache, dispatcher, cursor), not Transform._transform. None affected by this PR.

Hits inside Transform context: 0.

Semantic differential

Confirmed the change's behavior is observable with a focused test:

const t = new Transform({
  transform(chunk, enc, cb) {
    if (chunk.toString() === 'stop') cb(null, null);
    else cb(null, chunk);
  },
});
t.on('data', (d) => received.push(d));
t.write('a'); t.write('stop'); t.write('b'); t.end();

Node	Behavior
Baseline	received = ['a', 'b'] ('stop' swallowed)
PR	received = ['a'] + ERR_STREAM_PUSH_AFTER_EOF on the subsequent write('b')

So the change is a real semantic change — but we found no library in the tested universe that relies on it. Applications using callback(null, null) as an intentional no-op with subsequent writes would now see ERR_STREAM_PUSH_AFTER_EOF. This matches the documented intent of the callback (push the value passed) and the fix in this PR.

Conclusion & SemVer recommendation

• citgm-all: 0 regressions across 72 modules
• Extended stream subset: 0 regressions, 1 fix (JSONStream)
• Pattern audit: 0 Transform-context callback(null, null) call sites in the surveyed ecosystem

Given the pattern is not observed in the ecosystem but is a runtime-visible change, I'd suggest semver-minor with a release-note call-out — conservative given the theoretical crash path, but supported by the zero-breakage evidence. Happy to relabel semver-patch if collaborators think the release note is sufficient.

Alternatives considered

1. Land as semver-minor with release note (recommended): documents the fix, warns the ERR_STREAM_PUSH_AFTER_EOF path for code that mis-used the callback.
2. Deprecation warning cycle first: emit process.emitWarning on callback(null, null) for one release, then flip behavior. Overkill given 0 ecosystem hits.
3. Opt-in flag: { strictNullPush: true } option on Transform. Adds API surface without evidence of need.
4. semver-patch with release note: defensible given zero detected breakage; trades collaborator caution for simplicity.

Raw artifacts

Available on request — stdout logs, per-module TAP, and pattern-hits file can be uploaded to a Gist if useful.

Notes

• citgm-all ran in ~14 min per binary on an M4 Pro with 4-way concurrency; extended subset ran in ~10 min per binary.
• node-test-citgm on Jenkins would be a stronger datapoint; this local run is offered as a starting point. Happy to iterate if additional coverage is requested.

[galaxy4276]

This should have a pass of citgm before landing.

I expect a significant breakage in the ecosystem.

@mcollina I ran citgm locally on macOS (arm64) and have attached the results above. Please take a look whenever you get a chance — no rush at all. Hope you have a great day!