stream: end cannot throw ERR_STREAM_DESTROYED after destroy

[zero1five]

• Version: 12.1.0
• Platform: Darwin leaves 18.6.0 Darwin Kernel Version 18.6.0: Thu Apr 25 23:16:27 PDT 2019; root:xnu-4903.261.4~2/RELEASE_X86_64 x86_64

In objectMode mode, call end can receive null and also unable throw ERR_STREAM_DESTROYED.

const fs = require('fs');
const { createWriteStream } = fs;

const ws = createWriteStream('./test.js', { objectMode: true });

ws.on('close', () => {
  console.log('ws close');
});

ws.on('error', (error) => {
  throw error;
});

ws.destroy();

setTimeout(() => ws.end(null, 'utf-8', () => {
  console.log('ws ended');
}), 1000);

/**
 * output:
 *  ws close
 *  ws ended
 */

[ahmedHusseinF]

since the message is null, it isn't being written as a kind of optimization i guess.
look at the following snippet from Writable#end:

node/lib/_stream_writable.js

Lines 581 to 582 in 46d8af5

	if (chunk !== null && chunk !== undefined)
	this.write(chunk, encoding);

so do you have a use case where this behavior is wrong?

[zero1five]

@ahmedHusseinF i wanted point that:

writable.destroy([error])
After this call, the writable stream has ended and subsequent calls to write() or end() will result in an ERR_STREAM_DESTROYED error.

In my case, It should me throw ERR_STREAM_DESTROYED before console.log('ws ended').

writable.end(...[callback]) one of the callback is attached as a listener for the 'finish' event.

[jasnell]

Ping @nodejs/streams .. is there anything actionable here?

[ronag]

Yea, this example does show a problem with the end API:

• end(null) should throw ERR_STREAM_NULL_VALUES.
• end(undefined) should write undefined on object mode and throw ERR_INVALID_ARG_TYPE otherwise.

However, since it's an optional argument it becomes very confusing wether it should be interpreting the chunk value as something to send to write or whether it is simply an optional parameter which is not passed.

I don't see how we can solve this. Maybe if we check arguments.length but that would break code that passses nully to "opt out"? I would actually like if we could discourage use of end with data and doc deprecating that part. It has also other weird issues such as #34003.

[mcollina]

Making end(undefined) do anything different than end() seems extremely confusing.

[ronag]

Making end(undefined) do anything different than end() seems extremely confusing.

I agree. But given the current api and docs it should be possible to send undefined to an object stream. The signature says end([chunk]) and undefined is a valid chunk in object mode. There is a conflict/ambiguity in the api here.

This might be looking at it a bit too strictly. Not sure if this really matters in a practical sense. But it is weird/confusing.

[mcollina]

There is nothing we can do for this unless we create a massive breakage in the ecosystem. I propose to close.

[mafintosh]

Soft deprecating .end(data) is a good idea if you ask me.

[mcollina]

+1 for soft deprecating stream.end(data). I don't think we could hard-deprecate it ever :(.