stream: catch and forward error from dest.write

[jakecastelli]

Fixes: #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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.92%. Comparing base (b02cd41) to head (5c1a3e6).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55270      +/-   ##
==========================================
- Coverage   88.42%   87.92%   -0.50%     
==========================================
  Files         654      654              
  Lines      187852   187857       +5     
  Branches    36134    35832     -302     
==========================================
- Hits       166102   165170     -932     
- Misses      14989    15877     +888     
- Partials     6761     6810      +49     

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

... and 95 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.

  node/test/parallel/test-stream-pipeline.js

  Line 1357 in e79ae1b

  const w = new Writable({

  
  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

[mcollina]

This will likely break a lot of modules.

Many people (including myself) use object mode streams with strings or buffers. Even Node.js Core implies that, as we assumed this when we flipped Readable.from() to always be in object mode: Readable.from(['a', 'b']).pipe(fs.createWriteStream())

[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 #54945. That being said, I think the crash in #54945 is expected and "wanted". We make the process crash on programmer errors.

[mcollina]

I totally misunderstood this. I've left a few comments.

[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/

[benjamingr]

LGTM with green CITGM and semver-major

[jakecastelli]

Do you mind taking another quick look? @mcollina 🙏

[mcollina]

lgtm

I have a bad feeling this would be massively breaking.

[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/

[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/ 💛