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.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%)	⬆️

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

  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]

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

[mshima]

Any news about this PR?
In yo/jhipster we use a lot of streams and pipeline in objectMode.
The result is that the program hang silently with code 0 and no error is shown.

We couldn’t found a workaround. And I couldn’t confirm if this PR fixes the issue.

[jakecastelli]

Hi @mshima I have a few questions here:

1. what node version does you folks use?
2. do you have a minimal repro I can look into? or steps to repro the issue you mentioned

[mshima]

@jakecastelli thanks for your attention.

In my tests, node 22.4.0 works as expected.
The issue started in v22.5.0 and happens in v22.12.0.

Creating a minimal reproduction to match our use case is quite difficult, but seems likely this issue.
To reproduce we need to force an error in pipeline by creating an error:

npm install -g generator-jhipster@8.7.3
vim $(jhipster --install-path)/generators/angular/templates/jest.conf.js.ejs

replace:

const { pathsToModuleNameMapper } = require('ts-jest');

with (to make prettier fail):

const { pathsToModuleNameMapper } require('ts-jest');

Generate with offending file in an empty folder:

jhipster --defaults

[jakecastelli]

Thanks for providing some more info, I haven't run the script yet to install the pkg, could you confirm if the package name is correct? I searched on npm and found this https://www.npmjs.com/package/jhipster?activeTab=readme and is probably not the one I suppose to install

[mshima]

@jakecastelli sorry it’s generator-jhipster

[jakecastelli]

This is the error I've got, do you mind explaining a bit more on why you think this is related to the pipeline? And how do I test with node v22.4 vs v22.x, would also be good if you could also pin point me to the code in the generator-jhipster project that you think might be the issue. Cheers.

error

ERROR! ERROR! Error parsing file jest.conf.js: SyntaxError: Missing initializer in destructuring declaration. (1:34)
> 1 | const { pathsToModuleNameMapper } require('ts-jest');
    |                                  ^
  2 |
  3 | const {
  4 |   compilerOptions: { paths = {}, baseUrl = './' },

At: 1: const { pathsToModuleNameMapper } require('ts-jest');
2:
3: const {
4:   compilerOptions: { paths = {}, baseUrl = './' },
5: } = require('./tsconfig.json');
6: const environment = require('./webpack/environment');
7:
8: module.exports = {
9:   transformIgnorePatterns: ['node_modules/(?!.*\\.mjs$|dayjs/esm)'],
10:   resolver: 'jest-preset-angular/build/resolvers/ng-jest-resolver.js',
11:   globals: {
12:     ...environment,
13:   },
14:   roots: ['<rootDir>', `<rootDir>/${baseUrl}`],
15:   modulePaths: [`<rootDir>/${baseUrl}`],
16:   setupFiles: ['jest-date-mock'],
17:   cacheDirectory: '<rootDir>/target/jest-cache',
18:   coverageDirectory: '<rootDir>/target/test-results/',
19:   moduleNameMapper: pathsToModuleNameMapper(paths, { prefix: `<rootDir>/${baseUrl}/` }),
20:   reporters: [
21:     'default',
22:     ['jest-junit', { outputDirectory: '<rootDir>/target/test-results/', outputName: 'TESTS-results-jest.xml' }],
23:     ['jest-sonar', { outputDirectory: './target/test-results/jest', outputName: 'TESTS-results-sonar.xml' }],
24:   ],
25:   testMatch: ['<rootDir>/src/main/webapp/app/**/@(*.)@(spec.ts)'],
26:   testEnvironmentOptions: {
27:     url: 'https://jhipster.tech',
28:   },
29: };
30:
Error: Error parsing file jest.conf.js: SyntaxError: Missing initializer in destructuring declaration. (1:34)
> 1 | const { pathsToModuleNameMapper } require('ts-jest');
    |                                  ^
  2 |
  3 | const {
  4 |   compilerOptions: { paths = {}, baseUrl = './' },

At: 1: const { pathsToModuleNameMapper } require('ts-jest');
2:
3: const {
4:   compilerOptions: { paths = {}, baseUrl = './' },
5: } = require('./tsconfig.json');
6: const environment = require('./webpack/environment');
7:
8: module.exports = {
9:   transformIgnorePatterns: ['node_modules/(?!.*\\.mjs$|dayjs/esm)'],
10:   resolver: 'jest-preset-angular/build/resolvers/ng-jest-resolver.js',
11:   globals: {
12:     ...environment,
13:   },
14:   roots: ['<rootDir>', `<rootDir>/${baseUrl}`],
15:   modulePaths: [`<rootDir>/${baseUrl}`],
16:   setupFiles: ['jest-date-mock'],
17:   cacheDirectory: '<rootDir>/target/jest-cache',
18:   coverageDirectory: '<rootDir>/target/test-results/',
19:   moduleNameMapper: pathsToModuleNameMapper(paths, { prefix: `<rootDir>/${baseUrl}/` }),
20:   reporters: [
21:     'default',
22:     ['jest-junit', { outputDirectory: '<rootDir>/target/test-results/', outputName: 'TESTS-results-jest.xml' }],
23:     ['jest-sonar', { outputDirectory: './target/test-results/jest', outputName: 'TESTS-results-sonar.xml' }],
24:   ],
25:   testMatch: ['<rootDir>/src/main/webapp/app/**/@(*.)@(spec.ts)'],
26:   testEnvironmentOptions: {
27:     url: 'https://jhipster.tech',
28:   },
29: };

[mshima]

This is the error I've got

That's the expected error.
With node 22.12.0 the command just hang without showing the error. Like:

$ jhipster --defaults
.
.
.
identical src/test/java/com/mycompany/myapp/service/UserServiceIT.java
identical .lintstagedrc.cjs
$ echo $?
0

do you mind explaining a bit more on why you think this is related to the pipeline?

Looks related to pipeline transforms like:

      Duplex.from(async function* (files: AsyncGenerator<MemFsEditorFile>) {
        for await (const file of files) {
          if (isFilePending(file)) {
            yield file;
          }
        }
      }),

And how do I test with node v22.4 vs v22.x

I use n to switch between node versions.

n 22.12.0
jhipster --defaults

n 22.4.0
jhipster --defaults

would also be good if you could also pin point me to the code in the generator-jhipster project that you think might be the issue. Cheers.

It's not a single place, yeoman/generator#1577, SBoudrias/mem-fs-editor#282, SBoudrias/mem-fs#40.
With those changes the error is correctly shown in node 22.12.0.

[jakecastelli]

Hi @aduh95 I see you've helped with the CITGM result here - #55270 (comment) 🙏

I have to admit, I'm not entirely sure how to interpret the results. Is there any documentation that explains how to understand the CITGM results? For this PR, are there any user-land packages I should examine more closely?

[nodejs-github-bot]

Landed in fd8de67

[dario-piotrowicz]

@jakecastelli sorry, just out curiosity, is pipeine a typo? should it be pipeline?

[jakecastelli]

Yes, it is a typo, thanks for spotting it and PR welcome 👍

[jakecastelli]

I had an other look, my origin intention should be piping, I've created a quick PR here