Node: stream: `destroy(err)` predictability

[ronag]

The current official API for destroying is destroy(err). Where the user can supply a custom destroyer on the prototype through _destroy(err, cb) and where err comes from the call to destroy(err). The user can choose to ignore, replace, forward or modify err through the callback.

However, what is the actual vs expected behavior of this in regards to multiple calls to destroy(err)? The _destroy destroyer can only be invoked during the first call and any further calls to destroy(err) will currently bypass this and directly emit 'error'.

Examples

Example 1: We want to swallow errors. However, a second call to destroy(err) will actually emit an error.

const s = new Readable({
  read(),
  _destroy (err, cb) {
    // Swallow error.
    cb();
  }
});

s.on('error', common.mustNotCall()); // Fails. Invoked with 'error 2'.
s.destroy(new Error('error 1'));
s.destroy(new Error('error 2'));

Example 2: We want to (in some manner) modify the error.

{
  // Mutate
  const s = new Readable({
    read(),
    _destroy (err, cb) {
      if (err) {
        err.parent = this;
      }
      process.nextTick(cb, err);
    }
  });

  s.on('error', common.mustCall(err => {
    // Invoked with 'error 2'.
    assert.strictEqual(err.parent, s); // Fails. 'error 2'
    assert.strictEqual(err.message, 'error 1'); // Fails. 'error 2'
  }));
  s.destroy(new Error('error 1'));
  s.destroy(new Error('error 2'));
}

{
  // Replace
  const s = new Readable({
    read(),
    _destroy (err, cb) {
      process.nextTick(cb, new Error('error 3'));
    }
  });

  s.on('error', common.mustCall(err => {
    // Invoked with 'error 2'.
    assert.strictEqual(err.message, 'error 3'); // Fails. 'error 2'
  }));
  s.destroy(new Error('error 1'));
  s.destroy(new Error('error 2'));
}

Example 3: When the destroyer has fully completed and the stream is effectivly destructed an error can unexpectedly be injected.

let closed = false;
// Async
const s = new Readable({
  read(),
  _destroy (err, cb) {
    cb();
    closed = true;
  }
});

s.destroy();
assert.strictEqual(closed, true); // OK
// 'close' emits in nextTick
s.destroy(new Error('error 2')); // Fails. uncaught exception

This would fail even with https://github.com/nodejs/node/pull/29197 where errors are swallowed after 'close'.

Solutions

1. Originally in https://github.com/nodejs/node/pull/29197 we discussed that only the first call to destroy(err) is applied and any further calls are noop. This might be logical since we can only call _destroy once to apply any related logic. This would resolve the issues in all the examples above. The downside of this is in case destroy(err) is called after destroy() while destruction is still on-going, in which case the error would be swallowed. Due to this objection we came to the compromise of "don't emit 'error' after 'close'" to resolve the specific issue that PR was trying to resolve (i.e. when can no further 'error' events be emitted).

2. The first error always wins https://github.com/nodejs/node/pull/30982. Which would partly resolve Example 3 but not allow other error handling through _destroy.

3. Just leave things as they are and possibly document the edge cases.

Refs

• stream: don't emit 'error' after 'close' (https://github.com/nodejs/node/pull/29197)
• custom _destroy can swallow error? (https://github.com/nodejs/node/issues/30979)
• stream: first error wins and cannot be overriden (https://github.com/nodejs/node/pull/30982)

[ronag]

I would like to request guidance from the TSC in regards to how to resolve this and if any of the proposed solutions are satisfactory.

[Trott]

I would like to request guidance from the TSC in regards to how to resolve this and if any of the proposed solutions are satisfactory.

While I'm hesitant to speak for the TSC as a whole, I'd expect the TSC to prefer invested collaborators discuss the issue and try to reach consensus on how to handle this. @nodejs/streams

[Trott]

I would like to request guidance from the TSC in regards to how to resolve this and if any of the proposed solutions are satisfactory.

While I'm hesitant to speak for the TSC as a whole, I'd expect the TSC to prefer invested collaborators discuss the issue and try to reach consensus on how to handle this. @nodejs/streams

(Forgot to include that the @nodejs/tsc would hopefully get involved if there is an impasse.)

[lpinca]

There is a typo in example 3. let closed = true; should be let closed = false;.

[ronag]

I'd expect the TSC to prefer invested collaborators discuss the issue and try to reach consensus on how to handle this

Here is the current state of (un)consensus as far as I've been able to determine: https://github.com/nodejs/node/pull/30982#issuecomment-566124938

[sam-github]

Agree with @trott here. Its the streams experts who should weigh in.

I'm a user, not an expert, but I see nothing surprising about .destroy(); .destroy(err) not erroring. First destroy should win. Once a stream is being destroyed, its a bit late to start changing its state. I don't think its normally something node does (though the "too many event listeners" is a precedent), but I'd be OK with a warning being emitted for that usage (or at least, an internal debug message, so NODE_DEBUG can be used to figure whats happening).

Destroys aren't some kind of "queued event" wherein they all get queued up, then processed in order, and results seen in order. Its an orthogonal control flow (hopefully rare) -- orthogonal to the normal stream duplex ordered data followed by a FIN/end flow. They don't have well defined interplay, because in theory once .destroy() is called, the data flow no longer matters, other than it will terminate sometime before the destroy completes.

To have two destroy flows initiated is just a race condition.... exactly how they interact cannot be deterministic, other than that only the first one should win, and the second should do no harm.

Of course... if some popular package out there breaks when .destroy(); .destroy(err); doesn't error... then none of these theories matter. Breaking the ecosystem by making subtle changes to the behaviour of existing APIs isn't good. Then again, if the ecosystem is already subtly unstable because its relying on being lucky and undefined streams event orderings, making things better behaved might be worth it.

[ronag]

I'm a user, not an expert, but I see nothing surprising about .destroy(); .destroy(err) not erroring.

What I believe is the main objection against this can be found here https://github.com/nodejs/node/pull/29197#issuecomment-522721610

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.

[lpinca]

Of course... if some popular package out there breaks when .destroy(); .destroy(err); doesn't error... then none of these theories matter.

As written in #29197 it breaks ws. I have no idea if it is the only popular package that relies on current behavior. If consensus is reached with #29197 as it was originally thought, I will apply a workaround like the one suggested here: https://github.com/nodejs/node/pull/29197#issuecomment-530522593.

[Trott]

Here is the current state of (un)consensus as far as I've been able to determine: #30982 (comment)

For anyone who didn't click through, here's the comment:

Yes, you are right. As far as I see the current situation is:

• First error wins (this PR & https://github.com/nodejs/node/issues/30979). @mcollina -1
• First destroy() wins (as originally discussed in #29197). @lpinca -1
• Leave as is. @ronag -1

[mcollina]

Note that I’m also ok in leaving things as-is.

[ronag]

Another example of where this is relevant: https://github.com/nodejs/node/pull/31054#discussion_r361296216

[ronag]

The more I think about it the more I would prefer 1. Would resolve a lot of (for me) weird edge cases I keep running into. Though again I don't know how much it would break the ecosystem (other than ws). Would it make sense to make a PR and run a CITGM to try it?

[lpinca]

@ronag would #29197 as originally thought (all destroy() after the first are noop) solve all the edge cases we discussed? If so I would be ok with it after assessing possible ecosystem breakage beside ws (I've already made a patch for it).

[ronag]

@ronag would #29197 as originally thought (all destroy() after the first are noop) solve all the edge case we discussed?

I believe so.

[lpinca]

Ok, then I think it makes sense to restore https://github.com/nodejs/node/pull/29197 to its original state, rebase against master and check CITGM event though results may be misleading because current behavior may not be tested (for example it isn't in ws).

[ronag]

An update on solution 1, https://github.com/nodejs/node/pull/29197#issuecomment-569037445