Writeable stream .end() has paradoxical logic

[ronkorving]

• Version: 5.7.1
• Platform: all
• Subsystem: streams

It says here to ignore unnecessary end() calls. That means callbacks will not fire, even though end() is called. Should it not instead return either an error to the callback, or act as a noop before calling cb? The irony is that endWritable itself seems to act exactly like it may be called multiple times.

So which should it be? Right now, it seems like we have 2 different pieces of reasoning in the same class, and one simply overrules the other. Imho, we should get rid of one, and in my opinion, it should be the "ignore unnecessary end() calls" one, because when people pass a callback, they expect it to be called.

[Fishrock123]

cc @nodejs/streams

[mcollina]

I would say send a PR and we'll evaluate how many modules break :/. However all the writes and ends callbacks may not fire (as of now). This definitely improves the situation, but it's not the full solution.

[calvinmetcalf]

That logic in endWritable actually has to do with whether finishMaybe was able to finish the stream up or whether their were still pending async things.

This will actually only come up if end is called without any data, if it's called with data then it will through an error. In theory yes having it through an error would be good, it would be a breaking change which would likely be bad.

[jasnell]

ping @nodejs/streams

[mcollina]

I will put this into our next wg meeting agenda. So that we can decide what's the right path.

[mcollina]

I think the problem here is that the callback to end() might not be called. That looks like a bug to me.

[Trott]

Should this remain open?

[mcollina]

Yes, I think so.

[mcollina]

Ideally we would love to fix it, but it will cause too much breakages at this point in a lot of modules.
There is a lot of code out there that calls .end() multiple times, including .pipe().

You can use http://npm.im/pump or https://www.npmjs.com/package/end-of-stream to know if a stream has ended. We are porting those over in #13506.

cc @nodejs/streams