Skip to content

stream: fix multiple `destroy()` calls

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

Update: This PR has changed (again), see https://github.com/nodejs/node/pull/29197#issuecomment-569037445.

Previously destroy could be called multiple times causing inconsistent
and hard to predict behavior. Furthermore, since the stream _destroy
implementation can only be called once, the behavior of applying destroy
multiple times becomes unclear.

This changes so that only the first destroy() call is executed and any
subsequent calls are noops.

Update (via @Fishrock123): This PR has changed, see https://github.com/nodejs/node/pull/29197#issuecomment-530549797

Emitting 'error' after destroy() can surprise the user and lead to unexpected crashes.

In https://github.com/nodejs/node/pull/20077 we concluded that we should not emit any errors after abort() and then in https://github.com/nodejs/node/pull/28683 we agreed that abort() is destroy(). Hence, streams should not emit 'error' after destroy() for the same reasons as in https://github.com/nodejs/node/pull/20077.

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