Repository navigation
stream.finished behaviour change #29699
Description
Activity
I'm unsure what the appropriate course of action here is. I'd consider emitting
'close'before'end'as a bug. That's the whole thing withERR_STREAM_PREMATURE_CLOSE? Though, we've made the consequences of the bug more severe.This breaks modules that emit close immediately after calling stream.end() / push(null)
That sounds wrong though in general and is probably already broken in other more subtle ways.
If our strategy is to try to fix the ecosystem, I'm more than happy to try and help e.g. feross/multistream sort out these kind of bugs/issues (e.g. fix PRs).
The problem is that they call
destroy()directly afterpush(null). This will break insideReadable/Duplexin strange ways as well (not just infinished).https://github.com/feross/multistream/blob/master/index.js#L98
Also they are using a custom
destroyimplementation which further complicates things: https://github.com/feross/multistream/blob/master/index.js#L61. This implementation, unlike the native one, will emit'close'in the same tick which is the root cause.@mcollina This goes back a little bit with going around the streams framework. In this particular case:
- overriding
Readable/Writablemethods (e.g.destroyin this case). - manually emitting
'error'and'close'events. - manually destroying on end/finish (i.e. not using
autoDestroy).
I would suggest we update the documentation to more explicitly discourage this as it very easily breaks stream invariants and have consequences for the ecosystem.
- overriding
Custom destroy methods are everywhere in the ecosystem though, including using this pattern.
If we are going down the path of only supporting the latest stream ways we should remove all the legacy from the function, i.e. the request stream stuff https://github.com/nodejs/node/blob/master/lib/internal/streams/end-of-stream.js#L95
Custom destroy methods are everywhere in the ecosystem though, including using this pattern.
I do think we should support (but discourage) them within reason. We can't support every possible scenario. Especially not when it comes to what we otherwise consider as invalid.
Emitting
'close'before'end'will in my opinion break the ecosystem in both existing and future code if people assume the "correct" behaviour based on native node streams.If we are going down the path of only supporting the latest stream ways we should remove all the legacy from the function
I think we need to have a more balanced approach than that though. Those legacy stuff does not in any way hinder otherwise "correct" behaviour.
The problem is that they call destroy() directly after push(null).
My first thought is that breaking the stream ecosystem is not good, and we shouldn't do it.
But the specific thing above... I don't know, I might agree with @ronag on it. I ran into similar issues in the TLS testcases when updating to TLS1.3, because it changed the timing of the underlying stream activity.
As I understand what is happening in https://github.com/feross/multistream/blob/d63508c55e87d55a2ed6445ef79ba1c68881caad/index.js#L98-L99, push is async, its not at all guaranteed that the push will complete if delete() is called. The essential quality of delete() is that it does not wait, it doesn't participate in a deterministic event ordering, its just "as soon as possible".
write(); write(); end()guarantees the writes will happen in order, then the end.write(); write(); end(); destroy()makes no guarantees at all of the relative order of destroy. If the underlying stream is capable, it might literally do all that, in order, or it could in fact buffer the write/write/end, then destroy the stream so that none of the writes occur.Despite that its order isn't defined, with specific stream implementations, its behaviour will be predictable enough some code is relying on it, which is problematic.
I think we should revert that change.
Reverting the change will make other bugs possible such as:
const stream = require('stream') const assert = require('assert') const src = new stream.Readable() src.push('a') src.push(null) let data = '' const dst = new stream.Writable({ write (chunk, encoding, cb) { setTimeout(() => { data += chunk cb() }, 1000) } }) stream.pipeline( src, dst, err => { if (!err) { assert.strictEqual(data, 'a') // fails } } ) process.nextTick(() => { dst.destroy() })
We might already have such bugs in the ecosystem... but nobody notices it
The behaviour of example code above does not surprise me, I regard whether that data was written or not as undefined in the presence of a
.destroy()(at least, a destroy() that is not synchronized with the write completed callbacks).The behaviour can be made defined if
.destroy()is passed an Error argument.Another example:
http.createServer((req, res) => { stream.pipeline( fs.createReadStream(getPath(req)), res, // res does not emit 'error' if prematurely closed, instead it emits 'aborted'. err => { if (!err) { // Everything ok? Probably... but maybe not... depending on timing } } ) }).listen(0)
Or the corresponding example for client side (which is probably worse).
I can definitely see the argument @ronag is making here but I'm also super sensitive to breakage in the streams code. That is to say, I prefer the new behavior but don't want to break stuff. It's a bit wonky, but perhaps reverting the change, then adding it back behind a configuration option on the stream object would be a good approach. New implementations can make use of the new behavior by switching the option on, existing implementations can continue as they are. Later, we can make the choice of deprecating the existing behavior then eventually switching the default. Takes longer, but avoids breakage.
16 remaining items
The suggested fix is
semver-minor, so reverting beforehand may still be desirable.I believe the topics of this issue has been resolved.
Some recent changes to
stream.finishedlanded in Node master changes the behaviour of the function.Before (in Node 12) if a stream was about to emit end/finish but hadn't and emitted close it would not be treated as an error. In master this triggers a premature close.
It's due to the changes here.
https://github.com/nodejs/node/blob/master/lib/internal/streams/end-of-stream.js#L86
This breaks modules that emit
closeimmediately after calling stream.end() / push(null), for example the popular multistream module, https://github.com/feross/multistream.A simple test case looks like this:
We might want to revert those changes or at least document them.