Repository navigation
[streams] Releasing write() callbacks on error #1746
Description
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
Activity
[-]Releasing write() callbacks on error[/-][+][streams] Releasing write() callbacks on error[/+]mcollina commentedon May 20, 2015
this seems relevant, but it's related to the
Readableinterface nodejs/node-v0.x-archive#5920, originally reported by @kanongil.also cc @chrisdickinson
chrisdickinson commentedon May 20, 2015
@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 – aerrorBehavior: CallOne | CallAll | Discardoption 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 commentedon May 20, 2015
@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.
doc: clarify when writable.end callback is called
Trott commentedon Mar 10, 2016
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 commentedon Mar 10, 2016
@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
doc: clarify when writable.write callback is called
doc: clarify when writable.write callback is called
doc: clarify when writable.write callback is called
doc: clarify when writable.write callback is called
doc: clarify when writable.write callback is called
doc: clarify when writable.write callback is called
mcollina commentedon Jun 6, 2017
Closing this. It is pretty old, and probably it's might cause too much breakage to be worth fixing.