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 errors after destroy #29211

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 19, 2019

This one is probably going to be a bit controversial but please give it a chance. It's the continuation on some related but less controversial PR's (e.g. #29197)

Basically it tries to enforce the following rules:

  • Emit one error after destroy(err).
  • Don't emit any error after destroy() (including if destroy(err) is called afterwards).
  • Always pass error to callback in destroy(err, cb) or destroy(null, cb).

The idea is to try to avoid unexpectedly crashing the user's application or emit any unexpected errors (even if the user has error listeners registered). Most users (I believe?) assume that after destroy() no more errors will be emitted, i.e. the object is practically "killed".

In the case where an error is explicitly passed to destroy(err), it still makes sense to emit one error. Since the user has explicitly passed one.

If the user wants to handle any errors on the no error case they should pass a callback in destroy(null, cb). This would also probably require that the undocumented/internal callback API for destroy() is documented.

This is semver-major but I don't believe the chances of it breaking anything are very high.

The tests also need a little more polish (I'll do that if there is interest in going forward with this PR).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag
Copy link
Member Author

ronag commented Aug 19, 2019

ping @mcollina @lpinca

- Don't emit any error on destroy().
- Emit error on destroy(err).
- Always pass any error to callbak in destroy(err, cb).
@lpinca
Copy link
Member

lpinca commented Aug 19, 2019

Don't emit any error after destroy() (including if destroy(err) is called afterwards).

I don't think this is a good idea. See #29197 (comment)

@ronag ronag mentioned this pull request Aug 19, 2019
4 tasks
@ronag
Copy link
Member Author

ronag commented Aug 19, 2019

@lpinca @mcollina: I'll close this PR so we can continue the discussion in one place. I'll re-open depending on how the conversation goes.

@ronag ronag closed this Aug 19, 2019
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

Successfully merging this pull request may close these issues.

2 participants