Skip to content

stream: don't emit errors after destroy

Rodrigo Muino Tomonari requested to merge github/fork/nxtedition/stream-destroy-error into master

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. https://github.com/nodejs/node/pull/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

Merge request reports