Revert 5541300 and add coverage to pipeline

[jakecastelli]

#55096 introduced a regression which causes the callback in the pipeline not called when error is thrown in read from readable. Please note this is on the current main not released yet.

I think this would cause a massive breakage as I found this out through some user land applications / packages by using the nightly build. My suggestion is:

1. revert 5541300 (as it currently causes regression on nightly build)
2. adding a test coverage / some test coverages
3. let the origin author to reopen a PR and we run a CITGM to see if there are more potential breakages

Take this code snippet as repro:

const { Readable, pipeline, Duplex, Writable } = require("node:stream");

async function* myAsyncGenerator(ag) {
  for await (const data of ag) {
    yield data;
  }
}

const duplexStream = Duplex.from(myAsyncGenerator);

const r = new Readable({
  read() {
    this.push("data1\n");
    throw new Error("booom");
  },
});

const w = new Writable({
  write(chunk, encoding, callback) {
    callback();
  },
});

pipeline(r, duplexStream, w, (err) => {
  console.log("Error in pipeline:", err); // you won't be able to see this line as the cb never get called
});

This PR reverted 5541300 and added a new test coverage.

cc @matthieusieben

[nodejs-github-bot]

Review requested:

• @nodejs/streams

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (80e3ef3) to head (175b3aa).
Report is 43 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56278      +/-   ##
==========================================
- Coverage   88.55%   88.54%   -0.01%     
==========================================
  Files         657      657              
  Lines      190265   190220      -45     
  Branches    36541    36534       -7     
==========================================
- Hits       168483   168430      -53     
- Misses      14970    14971       +1     
- Partials     6812     6819       +7     

Files with missing lines	Coverage Δ
lib/internal/streams/duplexify.js	96.56% <100.00%> (-0.60%)	⬇️

... and 24 files with indirect coverage changes

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[matthieusieben]

Isn't this due to the fact that autoDestroy is not set in the readable options ?

apparently not.

[matthieusieben]

I adressed this issue in #56287. Streams are hard 😅

[jakecastelli]

Can we get this landed first?

First this is breaking on nightly.
Second, @matthieusieben can take time to work on opening a new PR with the origin change and his fix in #56287 in a single commit and we run CITGM. What do you folks think?

This PR is ready with a green CI. cc @nodejs/streams

[matthieusieben]

I agree that this is probably the best course of action

[mcollina]

lgtm

[mcollina]

@matthieusieben @nodejs/streams may i get another LGTM so we can land?

[nodejs-github-bot]

Landed in a9e65f6...5ea6fa7