stream: pass error on legacy destroy

[greguz]

While looking at readable-stream source, I've found a possible missed argument while emitting an error toward legacy streams.

• Include tests for any bug fixes or new features.
• Update documentation if relevant.
• Ensure that make lint passes.
• Ensure that make -j4 test (UNIX), or vcbuild test (Windows) passes.

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[mscdex]

This needs a test

[greguz]

@mscdex I came out (about tests) with this:

test/parallel/test-stream-pipeline.js

{
  // Mimics a legacy stream without the .destroy method
  class LegacyWritable extends Stream {
    write(chunk, encoding, callback) {
      callback();
    }
  }

  const writable = new LegacyWritable();
  writable.on('error', common.mustCall((err) => {
    assert.deepStrictEqual(err, new Error('stop'));
  }));

  pipeline(
    Readable.from({
      [Symbol.asyncIterator]() {
        return {
          next() {
            return Promise.reject(new Error('stop'));
          }
        };
      }
    }),
    writable,
    common.mustCall((err) => {
      assert.deepStrictEqual(err, new Error('stop'));
    })
  );
}

I have to say that this is a very specific edge case. The only way to trigger the bug is to use either finished or compose functions provided by the stream subsystem in combination with not-so-standard stream instances.

[benjamingr]

The test looks fine (it's indeed an edge case but the test is needed to make sure we don't cause a regression in the future)

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 51beb26