finish event of stream.Writable is not expectedly emitted

[cynron]

• Version: v8.0.0-pre/master branch, v6.9.5
• Subsystem: stream.Writable

Hi,

If _writableState.corked > 0 and _writev callback with an error, after calling writable.end(), finish event is not emitted.

Test Case:

const stream = require('stream');

const writable = new stream.Writable();

writable._write = (chunks, encoding, cb) => {
  cb(new Error('write test error'));
};

writable._writev = (chunks, cb) => {
  cb(new Error('writev test error'));
};

writable.on('finish', () => {
  console.error('finish'); /* it should print "finish", but not */
});

writable.on('prefinish', () => {
  console.error('prefinish');
});

writable.on('error', (er) => {
  console.error('error');
});

writable.cork();
writable.write('test');

setTimeout(function() {
    writable.end('test');
}, 10);

In onwriteError, finishMaybe should also be checked. a test patch for master works:

diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
index eedeb56..ccda379 100644
--- a/lib/_stream_writable.js
+++ b/lib/_stream_writable.js
@@ -330,9 +330,14 @@ function doWrite(stream, state, writev, len, chunk, encoding, cb) {
 function onwriteError(stream, state, sync, er, cb) {
   --state.pendingcb;
   if (sync)
-    process.nextTick(cb, er);
+    process.nextTick(after, er);
   else
+    after(er);
+
+  function after(er) {
     cb(er);
+    finishMaybe(stream, state);
+  }

   stream._writableState.errorEmitted = true;
   stream.emit('error', er);

Please confirm whether it is a bug? make a PR for this?

Thanks!

[cjihrig]

I'm not sure if this is a bug. You could probably make a case both ways. If we don't end up making a code change as a result of this issue, we should at least update the documentation.

[cynron]

Another testcase

finish is not emitted

// async_write.js
const stream = require('stream');
const writable = new stream.Writable();

writable._write = (chunks, encoding, cb) => {
  setTimeout(function() {
    cb(new Error('write test error'));
  }, 0); // _write async, defer callback
};

writable.on('finish', () => {
    console.error('finish'); // finish is not emitted
});

writable.on('error', (er) => {
    console.error('error');
});

writable.end('test');

finish is emitted

// sync_write.js
const stream = require('stream');
const writable = new stream.Writable();

writable._write = (chunks, encoding, cb) => {
  cb(new Error('write test error'));  // _write sync, callback immediately
};

writable.on('finish', () => {
    console.error('finish'); // finish is emitted 
});

writable.on('error', (er) => {
    console.error('error');
});
writable.end('test');

their behavior is inconsistent, after error occured,finish event should be either always emitted or never emitted. I think we should make it consistent and update the documentation to clarify this

[cjihrig]

We should probably be consistent. cc: @nodejs/streams

[mcollina]

@cynron would you like to fire a PR? I think this is an actual bug, good find.
I'm not 100% sure of the fix, but we can work on it!

@mafintosh what do you think?

[cynron]

@mcollina I'm sorry I don't know which is expected, always emitted or never emitted?

[mcollina]

@cynron I think it should emit 'finish'.

[mcollina]

Fixed in b153420.