rfc: standardize stream.destroy() and pipe

[mcollina]

A lot of modules, both internal (fs, net, http) and external (through2 and others) implement stream.destroy() and in fact I alway recommend using @mafintosh's pump to pipe streams. That module automatically calls destroy() if present, so no file descriptors are leaked in case of an error.

Given that it is a de-facto standard, we can document it in the stream API, and add a default implementation of it.

Maybe we can even go further and add a pump-equivalent in core, maybe as an option to pipe.

cc @nodejs/streams

[mcollina]

This is already coming up in the issues, see #4226.

[calvinmetcalf]

It could call a _destroy method and after that method called it's callback
it could shut down the stream, or maybe have a _destroyRead and
_destroyWrite method to impliment so duplex streams could have different
methods.

On Wed, Dec 23, 2015, 6:47 PM Matteo Collina [email protected]
wrote:

This is already coming up in the issues, see #4226
#4226.

—
Reply to this email directly or view it on GitHub
#4401 (comment).

[mcollina]

@calvinmetcalf 👍 I would like to know from everybody else if they like the idea.

[chrisdickinson]

Before we consider adding destroy, I'd like to have a writeup of what the purpose of destroy is and where it fits into the streams state machine to make sure we're all on the same page with what it is and what it should do. Ideally this writeup should address:

• What is a summary of the purpose of destroy?
• What calls destroy, under what circumstances?
• What events precede destroy, and what events succeed it?
• When is destroy appropriate for a user to call, if ever?
• What happens when destroy is not present on a stream?
• What happens when destroy throws?
• Does it work with Streams1 streams?
• What happens when streams with destroy are used with Streams2+ streams that aren't destroy-aware?

It's one of those things that's kind of half-documented right now. Streams1 uses it while streams2+ doesn't, so it's also half-in-use. I'd like to make sure we have a very clear idea of the state machine we're changing.

[alexjeffburke]

There are a couple of important things that come to my mind mind.

The first, and perhaps most importantly, would be clear semantics of the events that get fired when destroy() is called on a stream. The few times I've used the destroy method extreme care has been required with on() calls (in some cases defensively binding to both 'end' and 'close' for example) because it wasn't at all clear the order in which they will fire and which would surface any error condition.

Second, if the semantics of destroy() would be to immediately tear everything down (which to me is suggested from the name.. destroy() is definitive and perhaps could be considered slightly exceptional), will it do the right thing with downstream Streams? For example, once you've piped a stream to an HTTP client the only way to cancel that download is to write invalid null bytes to axe the request - I know this was done at a previous workplace and I believe is also handled by express.

[mcollina]

Regarding destroy, my idea is to standardize what is present both in core and in userland. Something that it is easily backportable.

What you mention "doing the right thing", si way more complicated and that's something we need to discuss. I would call that "Streams 4".

[alexjeffburke]

Yeah apologies I didn't mean to stray into blue sky territory! It was just my thinking through things somewhat aloud :)

I think this is worth doing, really dislike the thought some streams have this and others just won't. What you mentioned about back porting, that is an important concern - let's say you're working on a library, if you can write most of your code assuming this method and get down to a one line conditional to fill the gap that's likely a winner.

Btw, I know there are always concerns about moving streams forward, but if that's the case the way to allay those fears is small incremental changes and this seems a good candidate.

[mcollina]

Closing, as this is released in node 8.