stream.finished behaviour change

[mafintosh]

Some recent changes to stream.finished landed 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 close immediately after calling stream.end() / push(null), for example the popular multistream module, https://github.com/feross/multistream.

A simple test case looks like this:

const Multistream = require('multistream')
const fs = require('fs')

const m = new Multistream([
  fs.createReadStream(__filename)
])

m.resume()
stream.finished(m, function (err) {
  console.log('should not error:', err)
})

We might want to revert those changes or at least document them.

[mafintosh]

/cc @mcollina

[addaleax]

@ronag

[ronag]

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 with ERR_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).

[ronag]

The problem is that they call destroy() directly after push(null). This will break inside Readable/Duplex in strange ways as well (not just in finished).

https://github.com/feross/multistream/blob/master/index.js#L98

Also they are using a custom destroy implementation 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.

[ronag]

feross/multistream#46

[ronag]

@mcollina This goes back a little bit with going around the streams framework. In this particular case:

• overriding Readable/Writable methods (e.g. destroy in 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.

[mafintosh]

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

[ronag]

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.

[sam-github]

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.

[mcollina]

I think we should revert that change.

[ronag]

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

[sam-github]

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.