finish event of stream.Writable is not expectedly emitted

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!

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.

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

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

@cynron

@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?

@mcollina

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

@cynron

@cynron I think it should emit 'finish'.

Fixed in b153420.

stream: emit finish when using writev and cork

Navigation Menu

Repository navigation

`finish` event of stream.Writable is not expectedly emitted #11121

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

finish event of stream.Writable is not expectedly emitted #11121

Description

Activity

cjihrig commented on Feb 2, 2017

cynron commented on Feb 3, 2017

cjihrig commented on Feb 3, 2017

mcollina commented on Feb 3, 2017

cynron commented on Feb 4, 2017

mcollina commented on Feb 4, 2017

mcollina commented on May 26, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`finish` event of stream.Writable is not expectedly emitted #11121