stream.finished calls callback when writable stream still has pending callbacks

[watson]

Version

>=17.0.0 (verified all the way up to the latest 19.7.0)

Platform

Darwin watson-2.local 22.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:03:51 PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T6000 arm64

Subsystem

stream

What steps will reproduce the bug?

Run the following code and observe that the last assert throws because the stream still has a pending callback (because setImmediate hasn't called cb yet inside the streams write function:

const { strict: assert } = require('node:assert');
const { Duplex, finished } = require('node:stream');

const stream = new Duplex({
  write (chunk, enc, cb) {
    setImmediate(cb);
  }
});

stream.end('foo');

finished(stream, { readable: false }, (err) => {
  console.log('Finished!');
  assert.equal(err, undefined);
  assert.equal(stream._writableState.pendingcb, 0);
});

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

Always

What is the expected behavior?

I wouldn't expect stream.finished to call its callback if there's still pending callbacks on the writable side.

What do you see instead?

I would expect stream.finished to wait calling its callback until stream._writableState.pendingcb is 0.

Additional information

This is a follow up to issue #45281, which I had thought would fix the problem.

It's worth noting that this problem only occurs if it's a Duplex or Transform stream and only if finished is passed readable: false as an option.

Additionally, It might just be me who doesn't understand how stream.finished is supposed to behave in this instance, in which case I'm just using it wrong. But if so, I'm not sure how to know when there's no more pending callbacks?

[ErickWendel]

It really seems to be a bug! I'm troubleshooting it and will open a PR with the solution as soon as possible!

[ErickWendel]

I was digging a bit and it seems that when you call the .end function before the finished is attached it cannot handle it.

If I put a setTimeout so the .end is executed right after the finished is attached it works as expected:

const { strict: assert } = require('node:assert')
const { Duplex, finished } = require('node:stream')

const stream = new Duplex({
  write(chunk, enc, cb) {
    setImmediate(cb)
  }
})
setTimeout(() => stream.end('foo'), 200);

finished(stream, { readable: false }, (err) => {
  console.log('Finished!')
  assert.strictEqual(err, undefined)
  assert.strictEqual(stream._writableState.pendingcb, 0)
})

Gonna ping the team here so we might discuss if this is actually the expected behavior

EDIT: When looking at this piece and checking the pendingcb it's set to 1 (which means it's not finished) gonna inspect more but it seems to me to be some unexpected behavior

cc @nodejs/streams

[ronag]

This looks suspicious. Same problem on v19?

[ErickWendel]

This looks suspicious. Same problem on v19?

I've tested from source and v19.7.0. Both have the same output