Allow manipulating the generator in Duplex.from()#55096

matthieusieben · 2024-09-24T09:05:21Z

Fix for #55077

mcollina

lgtm with a few nits, good job

lib/internal/streams/duplexify.js

nodejs-github-bot · 2024-09-24T11:21:57Z

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

test/parallel/test-stream-duplex-from.js

lib/internal/streams/duplexify.js

codecov · 2024-09-24T14:25:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.00%. Comparing base (4efb7ae) to head (d660809).
Report is 122 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #55096 +/- ## ======================================= Coverage 87.99% 88.00% ======================================= Files 656 656 Lines 188988 189033 +45 Branches 35988 35995 +7 ======================================= + Hits 166302 166350 +48 + Misses 15848 15844 -4 - Partials 6838 6839 +1

Files with missing lines	Coverage Δ
lib/internal/streams/duplexify.js	`97.16% <100.00%> (+0.59%)`	⬆️

... and 29 files with indirect coverage changes

matthieusieben · 2024-09-24T19:32:42Z

~~Not sure how to fix the breaking test(s):~~

test-stream-compose-operator

=== release test-stream-compose-operator === Path: parallel/test-stream-compose-operator node:internal/process/promises:394 triggerUncaughtException(err, true /* fromPromise */); ^ AssertionError [ERR_ASSERTION]: The input did not match the regular expression /boom/. Input: 'AbortError: The operation was aborted' at process.processTicksAndRejections (node:internal/process/task_queues:105:5){generatedMessage: true, code: 'ERR_ASSERTION', actual: AbortError: The operation was aborted at destroyer (node:internal/streams/destroy:328:11) at node:internal/streams/duplexify:89:7 at asyncGenerator.return (node:internal/streams/duplexify:245:9) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async /Users/msi/Github/nodejs/node/test/parallel/test-stream-compose-operator.js:63:5 at async nextAsync (node:internal/streams/from:182:33){code: 'ABORT_ERR' }, expected: /boom/, operator: 'rejects' } Node.js v23.0.0-pre Command: out/Release/node /Users/msi/Github/nodejs/node/test/parallel/test-stream-compose-operator.js

Fixed thanks to @jazelly 🙏

ronag · 2024-09-25T05:20:54Z

@benjamingr This seems super hacky... wdyt?

lib/internal/streams/duplexify.js

matthieusieben · 2024-09-26T11:35:52Z

@ronag I agree. I don't usually like to monkey patch stuff but in this case I coudn't think of another way...

I tried using a try/catch/finally block inside the async generator to do the same. But since those block don't get called unless an iteration started (next() called at least once), that didn't work.

jasnell · 2024-09-28T15:07:49Z

test/parallel/test-stream-duplex-from.js

 }));
 }
+
+{


Nit: I'd really prefer if we could start adding a bit more documentation to each individual test that briefly describes what is expected and what that test is verifying.

Sorry but I don't have more time to work on this at the moment. Will definitely think about it the next time I open a PR.

jazelly · 2024-11-23T08:37:52Z

This PR needs a rebase

mcollina

lgtm

nodejs-github-bot · 2024-11-29T10:54:17Z

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

nodejs-github-bot · 2024-11-30T03:29:47Z

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

nodejs-github-bot · 2024-11-30T10:16:00Z

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

mcollina

lgtm

nodejs-github-bot · 2024-12-11T16:44:56Z

Commit Queue failed

- Loading data for nodejs/node/pull/55096 ✔ Done loading data for nodejs/node/pull/55096 ----------------------------------- PR info ------------------------------------ Title Allow manipulating the generator in Duplex.from() (#55096) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch matthieusieben:fix-55077 -> nodejs:main Labels stream, semver-minor, test, needs-ci Commits 3 - stream: handle generator destruction from Duplex.from() - stream: add tests for Duplex.from() - stream: properly destroy duplexified functions Committers 1 - Matthieu Sieben <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 24 Sep 2024 09:05:21 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55096#pullrequestreview-2496213343 ✔ - Jason Zhang (@jazelly): https://github.com/nodejs/node/pull/55096#pullrequestreview-2470700576 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-11-30T10:16:00Z: https://ci.nodejs.org/job/node-test-pull-request/63811/ - Querying data for job/node-test-pull-request/63811/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 55096 From https://github.com/nodejs/node * branch refs/pull/55096/merge -> FETCH_HEAD ✔ Fetched commits as a1d980c4e058..d66080905684 -------------------------------------------------------------------------------- [main 039174a4d9] stream: handle generator destruction from Duplex.from() Author: Matthieu Sieben <[email protected]> Date: Tue Sep 24 11:04:07 2024 +0200 2 files changed, 189 insertions(+), 7 deletions(-) [main 027294b9d9] stream: add tests for Duplex.from() Author: Matthieu Sieben <[email protected]> Date: Tue Sep 24 21:31:23 2024 +0200 2 files changed, 57 insertions(+), 3 deletions(-) [main c7e870905c] stream: properly destroy duplexified functions Author: Matthieu Sieben <[email protected]> Date: Thu Sep 26 13:25:26 2024 +0200 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- stream: handle generator destruction from Duplex.from() PR-URL: #55096
 Reviewed-By: Matteo Collina <[email protected]>
 Reviewed-By: Jason Zhang <[email protected]>
[detached HEAD fd5069920c] stream: handle generator destruction from Duplex.from()
 Author: Matthieu Sieben <[email protected]>
 Date: Tue Sep 24 11:04:07 2024 +0200
 2 files changed, 189 insertions(+), 7 deletions(-)
 Rebasing (3/6)
 Rebasing (4/6)
 Executing: git node land --amend --yes
 --------------------------------- New Message ----------------------------------
 stream: add tests for Duplex.from()
PR-URL: #55096
 Reviewed-By: Matteo Collina <[email protected]>
 Reviewed-By: Jason Zhang <[email protected]>
[detached HEAD e748795b16] stream: add tests for Duplex.from()
 Author: Matthieu Sieben <[email protected]>
 Date: Tue Sep 24 21:31:23 2024 +0200
 2 files changed, 57 insertions(+), 3 deletions(-)
 Rebasing (5/6)
 Rebasing (6/6)
 Executing: git node land --amend --yes
 --------------------------------- New Message ----------------------------------
 stream: properly destroy duplexified functions
Co-authored-by: Jason Zhang <[email protected]>
 PR-URL: #55096
 Reviewed-By: Matteo Collina <[email protected]>
 Reviewed-By: Jason Zhang <[email protected]>
[detached HEAD 3a4c553d64] stream: properly destroy duplexified functions
 Author: Matthieu Sieben <[email protected]>
 Date: Thu Sep 26 13:25:26 2024 +0200
 1 file changed, 1 insertion(+), 1 deletion(-)
 Successfully rebased and updated refs/heads/main.
ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/12280911806

matthieusieben · 2024-12-12T08:13:15Z

Hello maintainers,

I am not sure what commit-queue-failed means or how to fix it. Also, it appears that an approval from @ronag is still needed.

Would love to see this land 🙏

nodejs-github-bot · 2024-12-12T08:14:10Z

Landed in 5541300

PR-URL: #55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

Notable changes: crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 module: * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185 * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698 * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 stream: * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096 PR-URL: #56310

Notable changes: crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 module: * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185 * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698 * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 stream: * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096 PR-URL: TODO

Notable changes: crypto: * graduate WebCryptoAPI Ed25519 and X25519 algorithms as stable (Filip Skokan) #56142 doc: * stabilize util.styleText (Rafael Gonzaga) #56265 module: * (SEMVER-MINOR) add prefix-only modules to `module.builtinModules` (Jordan Harband) #56185 * (SEMVER-MINOR) only emit require(esm) warning under --trace-require-module (Joyee Cheung) #56194 * (SEMVER-MINOR) use synchronous hooks for preparsing in import(cjs) (Joyee Cheung) #55698 * (SEMVER-MINOR) implement module.registerHooks() (Joyee Cheung) #55698 report: * (SEMVER-MINOR) fix typos in report keys and bump the version (Yuan-Ming Hsu) #56068 sqlite: * (SEMVER-MINOR) aggregate constants in a single property (Edigleysson Silva (Edy)) #56213 src,lib: * (SEMVER-MINOR) stabilize permission model (Rafael Gonzaga) #56201 stream: * (SEMVER-MINOR) handle generator destruction from Duplex.from() (Matthieu Sieben) #55096 PR-URL: #56310

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 24, 2024

matthieusieben mentioned this pull request Sep 24, 2024
Stream not destroyed when piped in Duplex.from() writable #55077
Open

matthieusieben force-pushed the fix-55077 branch 2 times, most recently from 094e571 to c22a808Compare September 24, 2024 11:02

mcollina approved these changes Sep 24, 2024
View reviewed changes

lib/internal/streams/duplexify.js Outdated Show resolvedHide resolved
lib/internal/streams/duplexify.js Outdated Show resolvedHide resolved

mcollina requested a review from ronag September 24, 2024 11:12

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

avivkeller reviewed Sep 24, 2024
View reviewed changes

test/parallel/test-stream-duplex-from.js Outdated Show resolvedHide resolved
test/parallel/test-stream-duplex-from.js Outdated Show resolvedHide resolved
lib/internal/streams/duplexify.js Outdated Show resolvedHide resolved

avivkeller reviewed Sep 24, 2024
View reviewed changes

lib/internal/streams/duplexify.js Outdated Show resolvedHide resolved

avivkeller added the stream Issues and PRs related to the stream subsystem. label Sep 24, 2024

matthieusieben force-pushed the fix-55077 branch 6 times, most recently from 881baa0 to 12439c6Compare September 24, 2024 13:00

jazelly reviewed Sep 25, 2024
View reviewed changes

lib/internal/streams/duplexify.js Outdated Show resolvedHide resolved

jasnell reviewed Sep 28, 2024
View reviewed changes

mcollina approved these changes Nov 29, 2024
View reviewed changes

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

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

jazelly approved these changes Nov 30, 2024
View reviewed changes

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

mcollina approved these changes Dec 11, 2024
View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 11, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 11, 2024

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

nodejs-github-bot merged commit 5541300 into nodejs:mainDec 12, 2024
75 checks passed

matthieusieben deleted the fix-55077 branch December 12, 2024 08:19

targos pushed a commit that referenced this pull request Dec 13, 2024
stream: handle generator destruction from Duplex.from()
64a5fed
PR-URL: #55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

jakecastelli mentioned this pull request Dec 17, 2024
Revert 5541300 and add coverage to pipeline #56278
Closed

ronag added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Dec 17, 2024

matthieusieben mentioned this pull request Dec 17, 2024
stream: prevent dead lock when Duplex generator is "thrown" #56287
Open

github-actionsbot mentioned this pull request Dec 18, 2024
2024-12-19, Version 23.5.0 (Current) #56310
Merged

RafaelGSS mentioned this pull request May 1, 2025
2025-05-06, Version 24.0.0 (Current) #57609
Merged

 }));
 }
+{

Uh oh!

Allow manipulating the generator in Duplex.from()#55096

Allow manipulating the generator in Duplex.from() #55096

Uh oh!

Conversation

matthieusieben commented Sep 24, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecovbot commented Sep 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

matthieusieben commented Sep 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Sep 25, 2024

Uh oh!

Uh oh!

matthieusieben commented Sep 26, 2024

Uh oh!

jasnellSep 28, 2024

Choose a reason for hiding this comment

Uh oh!

matthieusiebenNov 29, 2024

Choose a reason for hiding this comment

Uh oh!

jazelly commented Nov 23, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 29, 2024

Uh oh!

nodejs-github-bot commented Nov 30, 2024

Uh oh!

nodejs-github-bot commented Nov 30, 2024• edited by jazelly Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 11, 2024

PR-URL: #55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

PR-URL: #55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

Co-authored-by: Jason Zhang <[email protected]> PR-URL: #55096 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>

[detached HEAD 3a4c553d64] stream: properly destroy duplexified functions Author: Matthieu Sieben <[email protected]> Date: Thu Sep 26 13:25:26 2024 +0200 1 file changed, 1 insertion(+), 1 deletion(-) Successfully rebased and updated refs/heads/main.

Uh oh!

matthieusieben commented Dec 12, 2024

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecovbot commented Sep 24, 2024•
edited
Loading

matthieusieben commented Sep 24, 2024•
edited
Loading

nodejs-github-bot commented Nov 30, 2024•
edited by jazelly
Loading

PR-URL: #55096
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>

PR-URL: #55096
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>

Co-authored-by: Jason Zhang <[email protected]>
PR-URL: #55096
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>

[detached HEAD 3a4c553d64] stream: properly destroy duplexified functions
Author: Matthieu Sieben <[email protected]>
Date: Thu Sep 26 13:25:26 2024 +0200
1 file changed, 1 insertion(+), 1 deletion(-)
Successfully rebased and updated refs/heads/main.