stream: catch and forward error from dest.write #55270

jakecastelli · 2024-10-05T05:20:49Z

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 · 2024-10-05T05:20:53Z

Review requested:

@nodejs/streams

lpinca · 2024-10-05T06:04:36Z

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 · 2024-10-05T06:06:24Z

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 · 2024-10-05T06:10:08Z

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 · 2024-10-05T07:19:11Z

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 · 2024-10-10T04:28:26Z

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 · 2024-10-10T07:50:18Z

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

ronag · 2024-10-10T07:51:01Z

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 · 2024-10-10T07:56:05Z

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

lib/internal/streams/readable.js

test/parallel/test-stream-pipe-objectmode-to-non-objectmode.js

jakecastelli · 2024-10-10T13:31:45Z

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 · 2024-10-10T13:40:42Z

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 · 2024-10-10T15:42:53Z

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 · 2024-10-10T16:39:18Z

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

jakecastelli · 2024-10-12T04:51:05Z

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 · 2024-10-12T08:16:41Z

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

jakecastelli · 2024-11-19T21:45:05Z

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

StefanStojanovic · 2024-11-20T09:37:32Z

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: nodejs/build#3963. I've also reported this to Microsoft via the developer community issue.

nodejs-github-bot · 2024-11-27T00:33:32Z

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

nodejs-github-bot · 2024-11-28T23:38:58Z

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

nodejs-github-bot · 2024-11-29T21:42:19Z

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

jakecastelli · 2024-11-30T09:28:56Z

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 · 2024-12-05T11:55:06Z

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 · 2024-12-10T23:10:51Z

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 · 2024-12-11T00:42:40Z

Hi @mshima I have a few questions here:

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

mshima · 2024-12-11T00:58:51Z

@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 [email protected]
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 · 2024-12-11T11:59:59Z

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 · 2024-12-11T12:01:59Z

@jakecastelli sorry it’s generator-jhipster

jakecastelli · 2024-12-11T12:16:33Z

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 · 2024-12-11T12:54:44Z

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 · 2024-12-18T01:49:33Z

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 · 2024-12-19T11:49:36Z

Landed in fd8de67

jakecastelli requested review from ronag and lpinca October 5, 2024 05:20

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Oct 5, 2024

jakecastelli force-pushed the fix-54945 branch from 76823e6 to 6dfdf7e Compare October 5, 2024 06:05

mcollina requested changes Oct 10, 2024

View reviewed changes

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 10, 2024

mcollina requested changes Oct 10, 2024

View reviewed changes

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

test/parallel/test-stream-pipe-objectmode-to-non-objectmode.js Show resolved Hide resolved

ronag removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 10, 2024

ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 12, 2024

jakecastelli force-pushed the fix-54945 branch 2 times, most recently from c01a0b9 to 52d26c2 Compare October 18, 2024 11:00

jakecastelli changed the title ~~stream: catch and re-throw objectMode incompatible error~~ stream: catch and forward error from dest.write Oct 18, 2024

jakecastelli force-pushed the fix-54945 branch from 52d26c2 to de9f68a Compare October 18, 2024 11:07

jakecastelli force-pushed the fix-54945 branch from 5c1a3e6 to 26b3e41 Compare November 19, 2024 21:44

StefanStojanovic mentioned this pull request Nov 20, 2024

Windows GitHub Actions changes required #55929

Closed

stream: catch and forward error from dest.write

b1c5b00

jakecastelli force-pushed the fix-54945 branch from 26b3e41 to b1c5b00 Compare November 26, 2024 21:54

jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2024

jakecastelli removed the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Dec 5, 2024

jasnell approved these changes Dec 10, 2024

View reviewed changes

This was referenced Dec 11, 2024

fix: replace Duplex with a Transform SBoudrias/mem-fs#40

Merged

fix: replace Duplex with Transform SBoudrias/mem-fs-editor#282

Merged

fix: replace Duplex with Transform yeoman/generator#1577

Merged

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 19, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 19, 2024

nodejs-github-bot merged commit fd8de67 into nodejs:main Dec 19, 2024
59 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: catch and forward error from dest.write #55270

stream: catch and forward error from dest.write #55270

jakecastelli commented Oct 5, 2024 •

edited

Loading

nodejs-github-bot commented Oct 5, 2024

lpinca commented Oct 5, 2024

jakecastelli commented Oct 5, 2024

lpinca commented Oct 5, 2024 •

edited

Loading

codecov bot commented Oct 5, 2024 •

edited

Loading

jakecastelli commented Oct 10, 2024 •

edited

Loading

mcollina left a comment

ronag commented Oct 10, 2024 •

edited

Loading

ronag commented Oct 10, 2024 •

edited

Loading

lpinca commented Oct 10, 2024

mcollina left a comment

jakecastelli commented Oct 10, 2024

ronag commented Oct 10, 2024 •

edited

Loading

lpinca commented Oct 10, 2024 •

edited

Loading

mcollina commented Oct 10, 2024

jakecastelli commented Oct 12, 2024

ronag commented Oct 12, 2024

jakecastelli commented Nov 19, 2024

StefanStojanovic commented Nov 20, 2024

nodejs-github-bot commented Nov 27, 2024

nodejs-github-bot commented Nov 28, 2024

nodejs-github-bot commented Nov 29, 2024 •

edited by jakecastelli

Loading

jakecastelli commented Nov 30, 2024

jakecastelli commented Dec 5, 2024

mshima commented Dec 10, 2024 •

edited

Loading

jakecastelli commented Dec 11, 2024

mshima commented Dec 11, 2024 •

edited

Loading

jakecastelli commented Dec 11, 2024

mshima commented Dec 11, 2024

jakecastelli commented Dec 11, 2024

mshima commented Dec 11, 2024 •

edited

Loading

jakecastelli commented Dec 18, 2024

nodejs-github-bot commented Dec 19, 2024

stream: catch and forward error from dest.write #55270

stream: catch and forward error from dest.write #55270

Conversation

jakecastelli commented Oct 5, 2024 • edited Loading

nodejs-github-bot commented Oct 5, 2024

lpinca commented Oct 5, 2024

jakecastelli commented Oct 5, 2024

lpinca commented Oct 5, 2024 • edited Loading

codecov bot commented Oct 5, 2024 • edited Loading

Codecov Report

jakecastelli commented Oct 10, 2024 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Oct 10, 2024 • edited Loading

ronag commented Oct 10, 2024 • edited Loading

lpinca commented Oct 10, 2024

mcollina left a comment

Choose a reason for hiding this comment

jakecastelli commented Oct 10, 2024

ronag commented Oct 10, 2024 • edited Loading

lpinca commented Oct 10, 2024 • edited Loading

mcollina commented Oct 10, 2024

jakecastelli commented Oct 12, 2024

ronag commented Oct 12, 2024

jakecastelli commented Nov 19, 2024

StefanStojanovic commented Nov 20, 2024

nodejs-github-bot commented Nov 27, 2024

nodejs-github-bot commented Nov 28, 2024

nodejs-github-bot commented Nov 29, 2024 • edited by jakecastelli Loading

jakecastelli commented Nov 30, 2024

jakecastelli commented Dec 5, 2024

mshima commented Dec 10, 2024 • edited Loading

jakecastelli commented Dec 11, 2024

mshima commented Dec 11, 2024 • edited Loading

jakecastelli commented Dec 11, 2024

mshima commented Dec 11, 2024

jakecastelli commented Dec 11, 2024

mshima commented Dec 11, 2024 • edited Loading

jakecastelli commented Dec 18, 2024

nodejs-github-bot commented Dec 19, 2024

jakecastelli commented Oct 5, 2024 •

edited

Loading

lpinca commented Oct 5, 2024 •

edited

Loading

codecov bot commented Oct 5, 2024 •

edited

Loading

jakecastelli commented Oct 10, 2024 •

edited

Loading

ronag commented Oct 10, 2024 •

edited

Loading

ronag commented Oct 10, 2024 •

edited

Loading

ronag commented Oct 10, 2024 •

edited

Loading

lpinca commented Oct 10, 2024 •

edited

Loading

nodejs-github-bot commented Nov 29, 2024 •

edited by jakecastelli

Loading

mshima commented Dec 10, 2024 •

edited

Loading

mshima commented Dec 11, 2024 •

edited

Loading

mshima commented Dec 11, 2024 •

edited

Loading