Repository navigation
rfc: standardize stream.destroy() and pipe #4401
Description
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
Activity
mcollina commentedon Dec 23, 2015
This is already coming up in the issues, see #4226.
calvinmetcalf commentedon Dec 24, 2015
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 notifications@github.com
wrote:
mcollina commentedon Dec 24, 2015
@calvinmetcalf 👍 I would like to know from everybody else if they like the idea.
chrisdickinson commentedon Dec 24, 2015
Before we consider adding
destroy, I'd like to have a writeup of what the purpose ofdestroyis 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:destroy?destroy, under what circumstances?destroy, and what events succeed it?destroyappropriate for a user to call, if ever?destroyis not present on a stream?destroythrows?destroyare used with Streams2+ streams that aren'tdestroy-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 commentedon Dec 26, 2015
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 commentedon Mar 30, 2016
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 commentedon Mar 30, 2016
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 commentedon Jun 27, 2017
Closing, as this is released in node 8.