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

@@ -401,3 +402,193 @@ function makeATestWritableStream(writeFunc) {
    assert.strictEqual(d.writable, false);
  }));
 }
+
+{


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

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

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

mcollina approved these changes Sep 24, 2024

View reviewed changes

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

lib/internal/streams/duplexify.js Outdated Show resolved Hide 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-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024

RedYetiDev reviewed Sep 24, 2024

View reviewed changes

test/parallel/test-stream-duplex-from.js Outdated Show resolved Hide resolved

test/parallel/test-stream-duplex-from.js Outdated Show resolved Hide resolved

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

RedYetiDev reviewed Sep 24, 2024

View reviewed changes

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

RedYetiDev 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

lib/internal/streams/duplexify.js Outdated Show resolved Hide 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-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024

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

jazelly approved these changes Nov 30, 2024

View reviewed changes

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

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

matthieusieben commented Sep 24, 2024

mcollina left a comment

nodejs-github-bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

matthieusieben commented Sep 24, 2024 •

edited

Loading

ronag commented Sep 25, 2024

matthieusieben commented Sep 26, 2024

jasnell Sep 28, 2024

matthieusieben Nov 29, 2024

jazelly commented Nov 23, 2024

mcollina left a comment

nodejs-github-bot commented Nov 29, 2024

nodejs-github-bot commented Nov 30, 2024

nodejs-github-bot commented Nov 30, 2024 •

edited by jazelly

Loading

mcollina left a comment

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.

matthieusieben commented Dec 12, 2024

nodejs-github-bot commented Dec 12, 2024

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

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

Conversation

matthieusieben commented Sep 24, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

matthieusieben commented Sep 24, 2024 • edited Loading

ronag commented Sep 25, 2024

matthieusieben commented Sep 26, 2024

jasnell Sep 28, 2024

Choose a reason for hiding this comment

matthieusieben Nov 29, 2024

Choose a reason for hiding this comment

jazelly commented Nov 23, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 29, 2024

nodejs-github-bot commented Nov 30, 2024

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

mcollina left a comment

Choose a reason for hiding this comment

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.

matthieusieben commented Dec 12, 2024

nodejs-github-bot commented Dec 12, 2024

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.