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

In writable stream, should we always set state.errorEmitted to true when executing stream.emit('error')? #18181

Closed
MoonBall opened this issue Jan 16, 2018 · 7 comments

Comments

@MoonBall
Copy link
Member

MoonBall commented Jan 16, 2018

I have read the source code of lib/_stream_writable.js. I have a doubt about the meaning of state.errorEmitted.

Why do we set state.errorEmitted to true in some situations of stream.emit('error'), but doesn't set it in other situations. Moreover, it doesn't achieve emitting 'error' event only one time. For example, as shown below, the code will emit 'error' event two times :

const { Writable } = require('stream');

w = new Writable();
w.on('error', error => {
  console.log('on error.', error);
});
w.write(null);
w.write(null);
@MoonBall
Copy link
Member Author

@mcollina

@mcollina
Copy link
Member

@MoonBall as a Node.js user, it means nothing. Internally it is used in some checks (in destroy()) to avoid doing certain things if the condition is met. This state variable is also used throughout core (which we should refactor according to #445).

Making the change you propose is hard, but if you want to take a stab at it I will be happy review it.

@MoonBall
Copy link
Member Author

MoonBall commented Jan 17, 2018

if it only work in destroy(), we can modify it's name or it's comments, so that we can better understand it. The comment(https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L144) is not appropriate.

@mcollina
Copy link
Member

It is not just destroy: https://github.com/nodejs/node/search?utf8=%E2%9C%93&q=errorEmitted&type=.

Ideally all of that should not be needed, would you like to investigate?

@MoonBall
Copy link
Member Author

ok. I try to do it.

@Trott
Copy link
Member

Trott commented Nov 25, 2018

/ping @MoonBall @mcollina Should this remain open?

@mcollina
Copy link
Member

I think this should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants