stream: always invoke callback before emitting error

Ensure the callback is always invoked before emitting
the error in both sync and async case.
PR-URL: #29293
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: ronag

doc/api/stream.md

@@ -571,7 +571,8 @@ The `writable.write()` method writes some data to the stream, and calls the
 supplied `callback` once the data has been fully handled. If an error
 occurs, the `callback`*may or may not* be called with the error as its
 first argument. To reliably detect write errors, add a listener for the
-`'error'` event.
+`'error'` event. If `callback` is called with an error, it will be called
+before the `'error'` event is emitted.
 
 The return value is `true` if the internal buffer is less than the
 `highWaterMark` configured when the stream was created after admitting `chunk`.

lib/_stream_writable.js

@@ -158,6 +158,11 @@ function WritableState(options, stream, isDuplex) {
 // Should .destroy() be called after 'finish' (and potentially 'end')
 this.autoDestroy=!!(options&&options.autoDestroy);
 
+// Indicates whether the stream has errored. When true all write() calls
+// should return false. This is needed since when autoDestroy
+// is disabled we need a way to tell whether the stream has failed.
+this.errored=false;
+
 // Count buffered requests
 this.bufferedRequestCount=0;
 
@@ -401,7 +406,7 @@ function writeOrBuffer(stream, state, isBuf, chunk, encoding, cb) {
 if(!ret)
 state.needDrain=true;
 
-if(state.writing||state.corked){
+if(state.writing||state.corked||state.errored){
 varlast=state.lastBufferedRequest;
 state.lastBufferedRequest={
  chunk,
@@ -420,7 +425,9 @@ function writeOrBuffer(stream, state, isBuf, chunk, encoding, cb) {
 doWrite(stream,state,false,len,chunk,encoding,cb);
 }
 
-returnret;
+// Return false if errored or destroyed in order to break
+// any synchronous while(stream.write(data)) loops.
+returnret&&!state.errored&&!state.destroyed;
 }
 
 functiondoWrite(stream,state,writev,len,chunk,encoding,cb){
@@ -437,18 +444,11 @@ function doWrite(stream, state, writev, len, chunk, encoding, cb) {
 state.sync=false;
 }
 
-functiononwriteError(stream,state,sync,er,cb){
+functiononwriteError(stream,state,er,cb){
 --state.pendingcb;
 
-if(sync){
-// Defer the callback if we are being called synchronously
-// to avoid piling up things on the stack
-process.nextTick(cb,er);
-}else{
-// The caller expect this to happen before if
-// it is async
-cb(er);
-}
+cb(er);
+// This can emit error, but error must always follow cb.
 errorOrDestroy(stream,er);
 }
 
@@ -465,9 +465,14 @@ function onwrite(stream, er) {
 state.length-=state.writelen;
 state.writelen=0;
 
-if(er)
-onwriteError(stream,state,sync,er,cb);
-else{
+if(er){
+state.errored=true;
+if(sync){
+process.nextTick(onwriteError,stream,state,er,cb);
+}else{
+onwriteError(stream,state,er,cb);
+}
+}else{
 // Check if we're actually ready to finish, but don't emit yet
 varfinished=needFinish(state)||stream.destroyed;
 
@@ -622,7 +627,7 @@ Object.defineProperty(Writable.prototype, 'writableLength', {
 functionneedFinish(state){
 return(state.ending&&
 state.length===0&&
-!state.errorEmitted&&
+!state.errored&&
 state.bufferedRequest===null&&
 !state.finished&&
 !state.writing);

lib/internal/streams/destroy.js

@@ -27,6 +27,10 @@ function destroy(err, cb) {
 constr=this._readableState;
 constw=this._writableState;
 
+if(w&&err){
+w.errored=true;
+}
+
 if((w&&w.destroyed)||(r&&r.destroyed)){
 if(cb){
 cb(err);
@@ -50,10 +54,12 @@ function destroy(err, cb) {
 this._destroy(err||null,(err)=>{
 constemitClose=(w&&w.emitClose)||(r&&r.emitClose);
 if(cb){
+// Invoke callback before scheduling emitClose so that callback
+// can schedule before.
+cb(err);
 if(emitClose){
 process.nextTick(emitCloseNT,this);
 }
-cb(err);
 }elseif(needError(this,err)){
 process.nextTick(emitClose ? emitErrorCloseNT : emitErrorNT,this,err);
 }elseif(emitClose){
@@ -91,6 +97,7 @@ function undestroy() {
 
 if(w){
 w.destroyed=false;
+w.errored=false;
 w.ended=false;
 w.ending=false;
 w.finalCalled=false;
@@ -110,6 +117,10 @@ function errorOrDestroy(stream, err) {
 constr=stream._readableState;
 constw=stream._writableState;
 
+if(w&err){
+w.errored=true;
+}
+
 if((r&&r.autoDestroy)||(w&&w.autoDestroy))
 stream.destroy(err);
 elseif(needError(stream,err))

test/parallel/test-http2-reset-flood.js

@@ -67,7 +67,10 @@ const worker = new Worker(__filename).on('message', common.mustCall((port) => {
 h2header.writeIntBE(1,0,3);// Length: 1
 h2header.writeIntBE(i,5,4);// Stream ID
 // 0x88 = :status: 200
-conn.write(Buffer.concat([h2header,Buffer.from([0x88])]));
+if(!conn.write(Buffer.concat([h2header,Buffer.from([0x88])]))){
+process.nextTick(writeRequests);
+break;
+}
 }
 }
 

test/parallel/test-stream-writable-destroy.js

@@ -16,6 +16,20 @@ const assert = require('assert');
 assert.strictEqual(write.destroyed,true);
 }
 
+{
+constwrite=newWritable({
+write(chunk,enc,cb){
+this.destroy(newError('asd'));
+cb();
+}
+});
+
+write.on('error',common.mustCall());
+write.on('finish',common.mustNotCall());
+write.end('asd');
+assert.strictEqual(write.destroyed,true);
+}
+
 {
 constwrite=newWritable({
 write(chunk,enc,cb){cb();}

test/parallel/test-stream-writable-write-cb-error.js

@@ -0,0 +1,58 @@
+'use strict';
+constcommon=require('../common');
+const{ Writable }=require('stream');
+constassert=require('assert');
+
+// Ensure callback is always invoked before
+// error is emitted. Regardless if error was
+// sync or async.
+
+{
+letcallbackCalled=false;
+// Sync Error
+constwritable=newWritable({
+write: common.mustCall((buf,enc,cb)=>{
+cb(newError());
+})
+});
+writable.on('error',common.mustCall(()=>{
+assert.strictEqual(callbackCalled,true);
+}));
+writable.write('hi',common.mustCall(()=>{
+callbackCalled=true;
+}));
+}
+
+{
+letcallbackCalled=false;
+// Async Error
+constwritable=newWritable({
+write: common.mustCall((buf,enc,cb)=>{
+process.nextTick(cb,newError());
+})
+});
+writable.on('error',common.mustCall(()=>{
+assert.strictEqual(callbackCalled,true);
+}));
+writable.write('hi',common.mustCall(()=>{
+callbackCalled=true;
+}));
+}
+
+{
+// Sync Error
+constwritable=newWritable({
+write: common.mustCall((buf,enc,cb)=>{
+cb(newError());
+})
+});
+
+writable.on('error',common.mustCall());
+
+letcnt=0;
+// Ensure we don't live lock on sync error
+while(writable.write('a'))
+cnt++;
+
+assert.strictEqual(cnt,0);
+}

test/parallel/test-wrap-js-stream-exceptions.js

@@ -16,4 +16,8 @@ const socket = new JSStreamWrap(new Duplex({
 })
 }));
 
-assert.throws(()=>socket.end('foo'),/Error:writeEPROTO/);
+socket.end('foo');
+socket.on('error',common.expectsError({
+type: Error,
+message: 'write EPROTO'
+}));