Skip to content

stream: destroy(err) predictability #31060

Closed
@ronag

Description

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 #29197 where errors are swallowed after 'close'.

Solutions

  1. Originally in stream: fix multiple destroy() calls #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 stream: first error wins and cannot be overriden #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

Metadata

Assignees

No one assigned

    Labels

    streamIssues and PRs related to the stream subsystem.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions