Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking chage with writable stream events #13812

Closed
alexpenev-s opened this issue Jun 20, 2017 · 6 comments
Closed

Breaking chage with writable stream events #13812

alexpenev-s opened this issue Jun 20, 2017 · 6 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@alexpenev-s
Copy link
Contributor

alexpenev-s commented Jun 20, 2017

In some cases writable streams emit an error event after having already emitted a finish event.
This is inconsistent with older versions, and also breaks code that removes all event listeners on the finish event.

Consider the js code below. In version v8.1.1 the output is:
finish
error

In node version v6.9.5 and v4.2.6 the output is:
error

uname -a
Linux xxxx 3.16.0-4-amd64 #1 SMP Debian 3.16.43-2 (2017-04-30) x86_64 GNU/Linux

var stream = require('stream');

var rs = new stream.Readable();
rs.push('ok');
rs.push(null);
rs._read = () => { };

var ws = new stream.Writable();

ws.on('finish', () => { console.log('finish'); });
ws.on('error', () => { console.log('error'); });

ws._write = (chunk, encoding, done) => {
    setImmediate(() => { done(new Error()); });
};
rs.pipe(ws);
@vsemozhetbyt vsemozhetbyt added the stream Issues and PRs related to the stream subsystem. label Jun 20, 2017
@mcollina mcollina self-assigned this Jun 21, 2017
@mcollina
Copy link
Member

I'm looking into this cc @nodejs/streams

@mcollina
Copy link
Member

This was introduced by b153420.

@nodejs/lts you might not want to backport that one for now nodejs/Release#230 cc @gibfahn.

@calvinmetcalf
Copy link
Contributor

just based on the description my first thought is that the actual issue is that in previous versions the error comes back before the finish while in current versions it finishes first

@mcollina
Copy link
Member

@calvinmetcalf that was my understanding, but no. In the previous released there was no 'finish' event.

@mcollina
Copy link
Member

@calvinmetcalf actually, that was the original problem that I fixed. The breakage comes because 'finish' happens before 'error'. I'm fixing that now.

mcollina added a commit to mcollina/node that referenced this issue Jun 21, 2017
When _write completes with an Error, 'finish' was emitted before
'error' if the callback was asynchronous. This commit restore the
previous behavior.
The logic is still less then ideal, because we call the write()
callback before emitting error if asynchronous, but after if
synchronous. This commit do not try to change the behavior.
This commit fixes a regression introduced by:
nodejs#13195.

Fixes: nodejs#13812
@mcollina mcollina added the confirmed-bug Issues with confirmed bugs. label Jun 21, 2017
@mcollina
Copy link
Member

Fixed by b443288.

mcollina added a commit that referenced this issue Jun 22, 2017
When _write completes with an Error, 'finish' was emitted before
'error' if the callback was asynchronous. This commit restore the
previous behavior.
The logic is still less then ideal, because we call the write()
callback before emitting error if asynchronous, but after if
synchronous. This commit do not try to change the behavior.
This commit fixes a regression introduced by:
#13195.

Fixes: #13812
PR-URL: #13850
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit that referenced this issue Jun 24, 2017
When _write completes with an Error, 'finish' was emitted before
'error' if the callback was asynchronous. This commit restore the
previous behavior.
The logic is still less then ideal, because we call the write()
callback before emitting error if asynchronous, but after if
synchronous. This commit do not try to change the behavior.
This commit fixes a regression introduced by:
#13195.

Fixes: #13812
PR-URL: #13850
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Calvin Metcalf <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants