stream: catch and forward error from dest.write

[jakecastelli]

Fixes: https://github.com/nodejs/node/issues/54945

Wanted to give a quick mention why checking object modes at the start of the pipe function wouldn't work (even though I liked the idea). Because if the chunk from the source stream is not object while source stream is in object mode and destination stream is not in object mode, it should still work.

Consider the following example:

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

const write = new Writable({
  write(data, enc, cb) {
    // do something with the data
    cb();
  },
});

write.on("error", (err) => {
  console.log(err);
});

Readable.from("hello hello").pipe(write);

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/streams

[lpinca]

Because if the chunk from the source stream is not object while source stream is in object mode and destination stream is not in object mode, it should still work

I don't think it should be supported. It is most likely a programmer error.

[jakecastelli]

I don't think it should be supported. It is most likely a programmer error.

I agree, but I am afraid this would break many user land packages.

[lpinca]

We could mark it as semver-major and run CITGM. I think it should be fixed in the "broken" packages and I don't think there are many of them.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.96%. Comparing base (3bb1d28) to head (b1c5b00). Report is 161 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #55270   +/-   ##
=======================================
  Coverage   87.95%   87.96%           
=======================================
  Files         656      656           
  Lines      188310   188315    +5     
  Branches    35963    35973   +10     
=======================================
+ Hits       165630   165645   +15     
+ Misses      15854    15848    -6     
+ Partials     6826     6822    -4     

Files with missing lines	Coverage Δ
lib/internal/streams/readable.js	96.20% <100.00%> (+0.01%)	:arrow_up:

... and 28 files with indirect coverage changes

[jakecastelli]

I've been thinking about it, and I think we should more carefully consider whether we should throw directly in pipe method or not.

The reasons being:

• Readable.from will have objectMode: true by default which means the destination stream has to have objectMode: true, and we have a few tests to demonstrate that this would fail e.g. https://github.com/nodejs/node/blob/e79ae1bf0c82489d80b5f3eb2bd9b089fdfafb36/test/parallel/test-stream-pipeline.js#L1357 The Writable would require objectMode: true
• This could potentially break many user land code - previously I mentioned user land package, without CITGM I wouldn't have a confident answer, however I have spoken to a few friends and they have had a scan in their projects and found they would need to add objectMode: true in many places (keep in mind, this could also be a very biased result)

To wrap up, what I am trying to say is that we may wanna consider this breaking change more carefully as all the Readable.from will default to objectMode: true and we support passing non-object as data when objectMode is true.

cc. @nodejs/streams

[ronag]

@mcollina I think you misunderstand this PR. It basically fixes an uncatchable error. Doesn't break anything as far as I can tell.

[ronag]

Though I do think this PR is a little over engineered. It's enough to catch the error and forward it "as is" to destroy.

[lpinca]

@mcollina at the moment this only catches the error thrown by dest.write() and call dest.destroy() with it, so I think it will not break anything. It only addresses https://github.com/nodejs/node/issues/54945. That being said, I think the crash in https://github.com/nodejs/node/issues/54945 is expected and "wanted". We make the process crash on programmer errors.

[jakecastelli]

Thank you for the review! 🙏 I appreciate the feedback and am happy to remove this error. As @ronag pointed out, it seems like I may have over-engineered it. My initial intention was to help developers understand the error better, but looking back, it appears I might've complicated things more than I've fixed.

Another point for discussion is whether we should catch the error and call destroy on the dest. As @lpinca noted that this is a programmatic error and that a crash is expected.

[ronag]

Another point for discussion is whether we should catch the error and call destroy on the dest. As @lpinca noted that this is a programmatic error and that a crash is expected.

I disagree with @lpinca on this one and would prefer a catchable error. The exception guarantee is understandable and it's possible to isolate the error from other working parts of a service, i.e. better to have a http server return 500 on a broken path than stop responding on all paths.

[lpinca]

The exception guarantee is understandable and it's possible to isolate the error from other working parts of a service, i.e. better to have a http server return 500 on a broken path than stop responding on all paths.

The error is already understandable (your suggestion is to call dest.destry() with the same error) and is not recoverable. If that error occurs, there is something wrong in the user code and not Node.js code.

I would also argue that if we want this behavior we should make writable.write() call writable.destroy() on invalid input instead of throwing an error.

[mcollina]

@ronag this change of behavior would be semver-major anyway.

[jakecastelli]

I'm still unsure about whether we should catch the error or proceed with a hard crash. Is there a similar example in Node.js core that we could use for comparison?

On the other hand, does those lines make sense?

diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js
index ac14b202b6..5225aa13f4 100644
--- a/lib/internal/streams/writable.js
+++ b/lib/internal/streams/writable.js
@@ -477,8 +477,9 @@ function _write(stream, chunk, encoding, cb) {
       chunk = Stream._uint8ArrayToBuffer(chunk);
       encoding = 'buffer';
     } else {
-      throw new ERR_INVALID_ARG_TYPE(
-        'chunk', ['string', 'Buffer', 'TypedArray', 'DataView'], chunk);
+      stream.destroy(new ERR_INVALID_ARG_TYPE(
+        'chunk', ['string', 'Buffer', 'TypedArray', 'DataView'], chunk));
+      return
     }
   }

or I should move what I've done in the catch block into dest.write function.

[ronag]

I think your PR was fine other than there is no need to convert it into another error.

[jakecastelli]

Would be great to have some reviews, no hard feeling if folks would like to block it @nodejs/streams

[mcollina]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3497/

[nodejs-github-bot]

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

[jakecastelli]

Do you mind taking another quick look? @mcollina 🙏

[ronag]

lgtm

I have a bad feeling this would be massively breaking.

I don't see how. Is it just a feeling or do you have some ideas in regards to how?

[mcollina]

I don't see how. Is it just a feeling or do you have some ideas in regards to how?

Bad feeling, but I don't see how.

Let's do another CITGM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3507/

main: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3508/

FAILURE: 22 failures in 3507 not present in 3508


┌────────────────────────┬───────────────────┬───────────────────────┬───────────────────────┬──────────────────────┬─────────────────────┬────────────────┬───────────────┐
│ (index)                │ 0                 │ 1                     │ 2                     │ 3                    │ 4                   │ 5              │ 6             │
├────────────────────────┼───────────────────┼───────────────────────┼───────────────────────┼──────────────────────┼─────────────────────┼────────────────┼───────────────┤
│ fedora-latest-x64      │ 'bcrypt-v5.1.1'   │ 'binary-split-v1.0.5' │ 'body-parser-v1.20.3' │ 'browserify-v17.0.1' │ 'bufferutil-v4.0.8' │ 'jest-v29.7.0' │ 'mime-v4.0.4' │
│ fedora-last-latest-x64 │ 'jest-v29.7.0'    │ 'mime-v4.0.4'         │                       │                      │                     │                │               │
│ rhel8-s390x            │ 'mime-v4.0.4'     │                       │                       │                      │                     │                │               │
│ aix72-ppc64            │                   │                       │                       │                      │                     │                │               │
│ alpine-latest-x64      │ 'jest-v29.7.0'    │ 'mime-v4.0.4'         │                       │                      │                     │                │               │
│ osx11-x64              │ 'mime-v4.0.4'     │                       │                       │                      │                     │                │               │
│ osx11                  │ 'mime-v4.0.4'     │                       │                       │                      │                     │                │               │
│ win-vs2022             │ 'resolve-v1.22.8' │                       │                       │                      │                     │                │               │
│ rhel8-x64              │ 'jest-v29.7.0'    │ 'mime-v4.0.4'         │                       │                      │                     │                │               │
│ debian12-x64           │ 'jest-v29.7.0'    │ 'mime-v4.0.4'         │                       │                      │                     │                │               │
│ ubuntu2204-64          │ 'jest-v29.7.0'    │ 'mime-v4.0.4'         │                       │                      │                     │                │               │
│ rhel8-ppc64le          │ 'mime-v4.0.4'     │                       │                       │                      │                     │                │               │
└────────────────────────┴───────────────────┴───────────────────────┴───────────────────────┴──────────────────────┴─────────────────────┴────────────────┴───────────────┘

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[jakecastelli]

Had some unrelated coverage failure on Windows, rebased and rerun everything

[StefanStojanovic]

Had some unrelated coverage failure on Windows, rebased and rerun everything

This error is related to the VS v17.12 which was released a week ago and has a compiler change making Node.js compilation impossible. I've already opened an issue to track this in build: https://github.com/nodejs/build/issues/3963. I've also reported this to Microsoft via the developer community issue.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[jakecastelli]

CI is green again, can I get another approval or can someone take another look (since I need to rebase to pass the GHA) 🙏

[jakecastelli]

This PR is technically ready to land, would you mind taking another quick look on CITGM @mcollina

Also need another approval since this PR needed a rebase