[streams] Releasing write() callbacks on error

[mcollina]

In case the streams errors, e.g. for a broken TCP socket, or an application error, all buffered write callbacks are never called, here is an example:

var Writable = require('stream').Writable
var inherits = require('util').inherits

function MyWritable (opts) {
  Writable.call(this, opts)
}

inherits(MyWritable, Writable)

MyWritable.prototype._write = function (chunk, enc, done) {
  console.log('_write', chunk.toString())
  setTimeout(function () {
    done(new Error('faulty one'))
  }, 500)
}

var stream = new MyWritable()

stream.write(new Buffer('Hello First'), 'utf8', print('hello'))
stream.write(new Buffer('Hello Second'), 'utf8', print('second'))

stream.on('error', function (err) {
  console.log('error received', err)
})

function print (value, err) {
  return function () {
    console.log('done', value, err)
  }
}

Here is the output of the above program:

_write Hello First
done hello [Error: faulty one]
error received [Error: faulty one]

The code I am using to fix this behavior is https://github.com/mcollina/aedes/blob/master/lib/client.js#L87-L91.

I am ok to submit a PR about this issue, if you think it is worth fixing.

cc @mafintosh

[mcollina]

this seems relevant, but it's related to the Readable interface nodejs/node-v0.x-archive#5920, originally reported by @kanongil.

also cc @chrisdickinson

[chrisdickinson]

@mcollina: I have a bit of trepidation about this change, since it's hard to estimate the amount of breakage this would cause in the ecosystem (in that we go from "only calling one callback" to "calling all callbacks in the queue".) This should only be a problem for folks who are manually calling .write, though, so it's hard to say whether this is innocuous or not! One approach would be to introduce it as a new option for writable streams – a errorBehavior: CallOne | CallAll | Discard option that defaults to "CallOne" but may be overridden by users.

@kanongil: this seems to address a different issue than nodejs/node-v0.x-archive#5920 – which was looking for a way to "save" an error for the end of the stream.

[mcollina]

@chrisdickinson That's the same though I had, but I thought that we might solve some leaks/bugs in other people code. I'm ok with a backward-compatible fix, as it is the safest bet. Basically this is an issue for everybody using stream without piping them, which is really advanced anyway (and I am only doing it for performance reasons). I'm not sure about the 'discard' option though, but it fits. If it's ok, then I will send a PR and we can discuss it there.

Regarding the breakage, we might get this is as a backward compatible thing, and maybe switch when a major release gets out. However, we should test this with some popular modules, just to see if it has any impact.

Is there a specific reason why it's 'CallOne' and not 'callOne'? I would have picked the latter if asked.

[Trott]

Is this being worked on/discussed elsewhere? (Trying to figure out if this needs to be closed or updated or anything. Looks like the basic situation may not have changed much so probably neither of those?)

[mcollina]

@Trott neither of those. I think this should be part of the discussion in how to improve error handling in streams.

What we can do today is to fix the doc https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback and report that the callback might not be called at all if the stream gets destroyed before the data could be flushed. On top of that, we are missing a clear explanation of what .destroy() does anyway.

cc @calvinmetcalf