streams: event clarification and improvements

[mafintosh]

This is more of a discussion issue, but have some thoughts on streams and the stream events we currently support that I wanted to write down.

On the issue tracker there are various issues where people are confused about stream events (such as #6083) but it would be good to get some clarification and perhaps improving the flow

Readable events

A readable stream has the following (confusing) events

• end
• error
• close

I continue to see confusion around the end event, even in Node core code. The end event simply means that the stream has gracefully ended AND there is not more data left in the buffer. The last part can be especially confusing that that means that end can fire way after a stream has actually closed down, or not at all, if there is data waiting in the buffer.

Your stream should not be emitting end if it had an error because it didn't close gracefully.

Both close and error used to be more or less unspecified in terms of core streams but we recently cleaned that up with the added destroy method (thanks for @mcollina and @calvinmetcalf for taking the lead on that).

Now when a stream is destroyed it will set a flag internally that will make it never call _read again and emit error (if there was an error passed) and a close event.

In terms of streams this means that a close event will only be emitted when a stream is destroyed.

I suggest the following. After the stream has fully ended and emitted end we should call .destroy internally to help users more easily control the life-cycle around streams.

That way the last event emitted is always close and if close is emitted with no end that is considered a non graceful end.

To help stream consumers, we should always be emitting errors on a stream using the .destroy(err) method, including in node core. That makes sure the correct error handling logic is called and events are emitted in an deterministic fashion.

Writable events

For writable streams the situation is similar. We have the following confusing events

• finish
• error
• close

Similar to readable streams finish means a graceful end and is emitted after all writes are flushed and the _final handler has been run (if there is one). Again I suggest we make the stream call .destroy() after finish to make sure close is always emitted and make it very clear that finish might not be emitted in non-graceful end scenarios.

Duplex streams

For duplex streams destroy() would only be called internally after both end and finish.

TL;DR

Use .destroy(err) in node core to signal stream errors instead of emitted an error, that makes error handling easy. Let's always emit close after end and finish to make userland error handling a lot easier.

Noting that as the author of most of the stream error-handling helpers on npm this close event change will have close to no userland regressions as that's how we expect streams to work anyway most of the time.

[mafintosh]

/cc @nodejs/streams

[mcollina]

I'd say.. go for it.

[ronag]

I am strongly in favour of this. close should always be the last event to be emitted. Also, the only event that should be allowed to fire after error should only be close.

Is there any way we can enforce/assert this behavior in the Stream base class?

[glebec]

Can I understand then that close is ALWAYS emitted now? If so, this is contrary to the current docs (10.3.0), which state that close:

Indicates that the underlying connection was terminated before response.end() was called or able to flush.

This seems to be related to an HTTP logging bug that I now have in glebec/volleyball#28 which is now logging (incorrectly) that there was an ungraceful end to every single HTTP response. What is the correct way to detect this state now?

[mcollina]

@glebec this is a proposal, it is not implemented yet.

[glebec]

@mcollina — something did change recently in Node w.r.t. the close event; didn't realize this thread wasn't addressing that change. Sorry for posting in the wrong thread. Regards, —G.

[ronag]

I believe all of the items discussed in this issue has been resolved.

There are still some minor inconsistencies and edge cases here and there in core where events are emitted out of order or not at all but we are slowly ironing these out.

Given this I'm closing this issues. Please re-open of you feel that something has not been addressed.