stream: fix multiple destroy() calls

[ronag]

Update: This PR has changed (again), see #29197 (comment).

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 #29197 (comment)

---

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

In #20077 we concluded that we should not emit any errors after abort() and then in #28683 we agreed that abort() is destroy(). Hence, streams should not emit 'error' after destroy() for the same reasons as in #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

[ronag]

@mcollina would be very glad to have this in order to further simplify #29192.

[ronag]

If this PR is rejected, my opinion is that we should not fix/change the behaviour for ClientRequest either... to keep things consistent.

[mcollina]

LGTM

[mcollina]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1964/

[ronag]

@Trott: That travis failure is weird...?

[Trott]

CITGM results look worse than they are because AIX ran out of space? (/ping @nodejs/build @nodejs/citgm to check that I'm not reading the results wrong.)

[lpinca]

I'm 👎 on this. The reason is that it will make valid errors to be swallowed. For example consider this case:

1. stream.destroy() is called without error but does not complete because the _destroy() implementation is waiting for the callback to be called.
2. Some error occurs while we are waiting for _destroy() callback. That error is handled by calling stream.destroy(error).
3. Error is swallowed.

[ronag]

@lpinca: I'm of the opposite opinion. I'd rather not crash the user if he doesn't expect the error. I've had a few crashes in production due to unexpected errors I don't care about. I really have trouble with our junior developers not considering this behaviour.

However, if we don't do this, would you agree atleast that we do not do the referenced change (the swallow error part) in ClientRequest either. I'd like it to at least be consistent.

I'm further arguing the case here, #29211.

EDIT: I'm inserting the relevant parts in this thread to keep the conversation in one place:

• 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.

[lpinca]

However, would you agree then that we do not do the referenced change in ClientRequest either then

• http: ignore socket errors after req.abort #20077 (comment)
• http: ignore socket errors after req.abort #20077 (comment)
• http: ignore socket errors after req.abort #20077 (comment)
• http: ignore socket errors after req.abort #20077 (comment)

Yes 😄

I have code that relies on current behavior of emitting errors if _destroy() callback is not called: https://github.com/websockets/ws/blob/7.1.2/lib/stream.js#L82

[ronag]

Yes 😄

Let's see if we can get any other feedback in the opposite direction. Otherwise I will update and close PR's accordingly (unless anyone objects).

[ronag]

@Trott: Can we put this on the TSC agenda? My main problem is consistency, e.g. in pipeline, finished we do swallow errors. I'm more than happy to put in the required work in either direction. As long as we have some kind of thought through and consistent guideline/best practice/rule.

Another thing to keep in mind is that with destroyImpl and errorEmitted, we do swallow errors when we have more than 1.

The question is how do we handle errors in relation to destroy? The current rule is as far as I can tell:

• Only emit one error after destroy().
• Do not emit any error after pipeline(), finished() or Symbol.asyncGenerator.

Should probably explicitly document this somewhere? (The finished & pipeline already have doc PRs since before).

I changed my mind

[mcollina]

pipeline and finished are external to the stream classes, and they ensure that the process will not crash if 'error' is emitted. Changing the behavior of destroy() is totally different and the current combination works (pipeline/finished swallowing vs stream emitting error multiple times).

[mcollina]

@ronag can you please provide a clearer questions to the TSC? Something that can boil down to a "yes/no" decision.

[ronag]

Something that can boil down to a "yes/no" decision.

What I'm asking for is a set of bullet points on how destroy should behave in terms of errors.

Should it be:

• Emit max one error after destroy().

This is what stream destroy currently enforces.

Or should we be even more "helpful":

• Emit max 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).

See #29211 for an example how this could be implemented.

Or something else?

So I guess the "yes/no" question is: should we avoid (from the users perspective) "unexpected" errors after destroy()?

[ronag]

For context this is what we use internally for all destroy calls (for me this whole thing has been about getting rid of this helper).

const once = require('once')
const noop = () => {}
// If you want to know about errors, pass callback.
module.exports = function destroy (self, err, callback) {
  callback = callback ? once(callback) : null
  // Ensure no uncaught exception if we don't care about errors.
  self.on('error', callback || noop)
  if (callback) {
    self
      // node doesn't always implement destroy with callback
      .on('close', callback)
      // node doesn't always emit 'close'
      // use nextTick and "hope" 'close' is emitted by then.
      .on('end', () => process.nextTick(callback))
  }
  if (typeof self.abort === 'function') {
    // TODO: err is lost
    self.abort() // Fix for ClientRequest.
  } else {
    self.destroy(err, callback)
  }
}

[lpinca]

Making my 👎 explicit so this is not landed by mistake.

[ronag]

@Trott: maybe a WIP label (similar to #29205)? Or is tsc-agenda enough?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29441/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29444/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29446/

[ronag]

CI: yellow
CITGM: Known failures with citgm.eslint-plugin-jest-v23.8.0 and citgm.resolve-v1.15.1

[ronag]

@Trott: second opinion on CI & CITGM? I think yellow CI can land?

[Trott]

I think yellow CI can land?

Yes, yellow CI can land.

[ronag]

Landed in 311e12b

[kanongil]

This is a nice change, but I am missing a history note about the change.

Based on the current documentation, I called destroy(err) expecting it to noop, which will emit another error when using node 12 / readable-streams.