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 methodclassLegacyWritableextendsStream{write(chunk,encoding,callback){callback();}}constwritable=newLegacyWritable();writable.on('error',common.mustCall((err)=>{assert.deepStrictEqual(err,newError('stop'));}));pipeline(Readable.from({[Symbol.asyncIterator](){return{next(){returnPromise.reject(newError('stop'));}};}}),writable,common.mustCall((err)=>{assert.deepStrictEqual(err,newError('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