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

stream: don't emit 'error' on non destructive errors? #29861

Closed
ronag opened this issue Oct 6, 2019 · 4 comments
Closed

stream: don't emit 'error' on non destructive errors? #29861

ronag opened this issue Oct 6, 2019 · 4 comments
Labels
discuss Issues opened for discussions and feedbacks. stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Oct 6, 2019

We have some errors such as ERR_STREAM_WRITE_AFTER_END, ERR_STREAM_DESTROYED, ERR_STREAM_ALREADY_FINISHED, ERR_INVALID_ARG_TYPE etc. that are not due to internal stream errors but more along the line of external usage errors.

However, given current practice we always call errorOrDestroy (or along similar lines) for all errors, even though these errors do not in any way bring the stream into erroneous state, e.g.

writable.end();
// Incorrect usage but the stream could still properly finish.
writable.write('moredata', err => {
  console.log('usage error');
});
writable.on('error', err => {
  console.log('internal error'):
});

I would like to propose that these kind of errors do not emit 'error' nor destroy(err) the instance, if a callback is provided in the method call that caused the error.

I believe this would make things easier in terms of what users expect while at the same time not swallowing errors per se.

@ronag
Copy link
Member Author

ronag commented Oct 6, 2019

@lpinca: Thoughts or comments?

@ronag ronag changed the title stream: emit 'error' on non destructive errors? stream: don't emit 'error' on non destructive errors? Oct 6, 2019
@lpinca
Copy link
Member

lpinca commented Oct 6, 2019

Can you please elaborate on the advantages? I'm not sure I understand.

@lpinca lpinca added discuss Issues opened for discussions and feedbacks. stream Issues and PRs related to the stream subsystem. labels Oct 6, 2019
@ronag
Copy link
Member Author

ronag commented Oct 6, 2019

I thought I had a good example of this but while writing it down it became obvious that if this is important I can just do the check in user space.

I need to give things a bit more thought before opening an issue. Thanks for the reply and sorry to waste your time.

@ronag ronag closed this as completed Oct 6, 2019
@lpinca
Copy link
Member

lpinca commented Oct 6, 2019

No problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants