Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
stream: Writable.end(chunk) after end is an error
Browse files Browse the repository at this point in the history
Calling end(data) calls write(data).  Doing this after end should
raise a 'write after end' error.

However, because end() calls were previously ignored on already
ended streams, this error was confusingly suppressed, even though the
data never is written, and cannot get to the other side.
  • Loading branch information
isaacs committed Mar 3, 2013
1 parent 63edde0 commit 5222d19
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 12 deletions.
31 changes: 19 additions & 12 deletions lib/_stream_writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,10 +304,6 @@ Writable.prototype._write = function(chunk, cb) {
Writable.prototype.end = function(chunk, encoding, cb) {
var state = this._writableState;

// ignore unnecessary end() calls.
if (state.ending || state.ended || state.finished)
return;

if (typeof chunk === 'function') {
cb = chunk;
chunk = null;
Expand All @@ -317,17 +313,28 @@ Writable.prototype.end = function(chunk, encoding, cb) {
encoding = null;
}

state.ending = true;
if (chunk)
this.write(chunk, encoding, cb);
else if (state.length === 0 && !state.finishing && !state.finished) {
this.write(chunk, encoding);

// ignore unnecessary end() calls.
if (!state.ending && !state.ended && !state.finished)
endWritable(this, state, !!chunk, cb);
};

function endWritable(stream, state, hadChunk, cb) {
state.ending = true;
if (!hadChunk &&
state.length === 0 &&
!state.finishing) {
state.finishing = true;
this.emit('finish');
stream.emit('finish');
state.finished = true;
if (cb) process.nextTick(cb);
} else if (cb) {
this.once('finish', cb);
}

if (cb) {
if (state.finished || state.finishing)
process.nextTick(cb);
else
stream.once('finish', cb);
}
state.ended = true;
};
16 changes: 16 additions & 0 deletions test/simple/test-stream2-writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -311,3 +311,19 @@ test('duplexes are pipable', function(t) {
assert(!gotError);
t.end();
});

test('end(chunk) two times is an error', function(t) {
var w = new W();
w._write = function() {};
var gotError = false;
w.on('error', function(er) {
gotError = true;
t.equal(er.message, 'write after end');
});
w.end('this is the end');
w.end('and so is this');
process.nextTick(function() {
assert(gotError);
t.end();
});
});

3 comments on commit 5222d19

@bnoordhuis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac, this commit is breaking a lot of tests.

@isaacs
Copy link
Author

@isaacs isaacs commented on 5222d19 Mar 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Wonder how I missed that.

Rehashed in #4904

@Somebi
Copy link

@Somebi Somebi commented on 5222d19 Mar 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have posted issue about that long ago. Stream pipe method is not working properly.

Please sign in to comment.