custom _destroy can swallow error?

[ronag]

Was looking through the stream destroy tests and noticed this. Is this something we want? I would expect that the error would not get swallowed? i.e. I would expect the original/first error to always propagate.

  const write = new Writable({
    write(chunk, enc, cb) { cb(); },
    destroy: common.mustCall(function(err, cb) {
      assert.strictEqual(err, expected);
      cb();
    })
  });

  const expected = new Error('kaboom');

  write.on('finish', common.mustNotCall('no finish event'));
  write.on('close', common.mustCall());

  // Error is swallowed by the custom _destroy
  write.on('error', common.mustNotCall('no error event'));

  write.destroy(expected);
  assert.strictEqual(write.destroyed, true);

[ronag]

ping @lpinca

[lpinca]

It is done on purpose so I think it's ok.

[ronag]

@lpinca: then how about the case when destroy(err) is called again? Then it won't be swallowed which is a bit confusing if we are supposed to be able to suppress errors but it only works sometimes.

  const write = new Writable({
    write(chunk, enc, cb) { cb(); },
    destroy: common.mustCall(function(err, cb) {
      assert.strictEqual(err, expected);
      cb();
    })
  });

  const expected = new Error('kaboom');

  write.on('finish', common.mustNotCall('no finish event'));
  write.on('close', common.mustCall());

  // Error is swallowed by the custom _destroy
  write.on('error', common.mustNotCall('no error event'));

  write.destroy(expected);
  write.destroy(expected); // No longer swallowed... **HERE**
  assert.strictEqual(write.destroyed, true);

[lpinca]

Agreed, not sure how to fix that.

[ronag]

Keep it simple and don’t allow suppressing errors? Or is that too significant?

[lpinca]

I would be ok with that.