Allow manipulating the generator in Duplex.from() by matthieusieben · Pull Request #55096 · nodejs/node · GitHub

lgtm with a few nits, good job

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

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

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 🙏

@benjamingr This seems super hacky... wdyt?

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

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.

This PR needs a rebase

lgtm

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

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

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

lgtm

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 <matthieu.sieben@gmail.com> PR-URL: https://github.com/nodejs/node/pull/55096 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55096 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com> -------------------------------------------------------------------------------- ℹ 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 <matthieu.sieben@gmail.com> 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 <matthieu.sieben@gmail.com> 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 <matthieusieben@users.noreply.github.com> 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 <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>

[detached HEAD fd5069920c] stream: handle generator destruction from Duplex.from()
Author: Matthieu Sieben <matthieu.sieben@gmail.com>
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 <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>

[detached HEAD e748795b16] stream: add tests for Duplex.from()
Author: Matthieu Sieben <matthieu.sieben@gmail.com>
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 <xzha4350@gmail.com>
PR-URL: #55096
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>

[detached HEAD 3a4c553d64] stream: properly destroy duplexified functions
Author: Matthieu Sieben <matthieusieben@users.noreply.github.com>
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

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 🙏

Landed in 5541300

PR-URL: #55096 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>

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 c22a808 Compare September 24, 2024 11:02

mcollina approved these changes Sep 24, 2024

View reviewed changes

Comment thread lib/internal/streams/duplexify.js Outdated

Comment thread lib/internal/streams/duplexify.js Outdated

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-actions Bot 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

Comment thread test/parallel/test-stream-duplex-from.js Outdated

Comment thread test/parallel/test-stream-duplex-from.js Outdated

Comment thread lib/internal/streams/duplexify.js Outdated

avivkeller reviewed Sep 24, 2024

View reviewed changes

Comment thread lib/internal/streams/duplexify.js Outdated

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 12439c6 Compare September 24, 2024 13:00

jazelly reviewed Sep 25, 2024

View reviewed changes

Comment thread lib/internal/streams/duplexify.js Outdated

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-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024

github-actions Bot 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:main Dec 12, 2024

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 <matteo.collina@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>

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-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-actions Bot 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

maruthang mentioned this pull request Apr 25, 2026

stream: destroy duplex on early return in Duplex.from(asyncFn) #62953

Open

Allow manipulating the generator in Duplex.from() by matthieusieben · Pull Request #55096 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

Allow manipulating the generator in Duplex.from()#55096nodejs-github-bot merged 3 commits intonodejs:mainnodejs/node:mainfrom matthieusieben:fix-55077matthieusieben/node:fix-55077Copy head branch name to clipboard

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!

codecov Bot 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!

jasnell Sep 28, 2024

Choose a reason for hiding this comment

Uh oh!

matthieusieben Nov 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 <matteo.collina@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>

PR-URL: #55096 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>

Co-authored-by: Jason Zhang <xzha4350@gmail.com> PR-URL: #55096 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>

[detached HEAD 3a4c553d64] stream: properly destroy duplexified functions Author: Matthieu Sieben <matthieusieben@users.noreply.github.com> 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!

nodejs-github-bot commented Dec 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Allow manipulating the generator in Duplex.from()#55096
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
matthieusieben:fix-55077

codecov Bot 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 <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>

PR-URL: #55096
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>

Co-authored-by: Jason Zhang <xzha4350@gmail.com>
PR-URL: #55096
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>

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