Allow manipulating the generator in Duplex.from()
Fix for #55077
CI: https://ci.nodejs.org/job/node-test-pull-request/62727/
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
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%) |
:arrow_up: |
~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.
This PR needs a rebase
CI: https://ci.nodejs.org/job/node-test-pull-request/63772/
Just rebased and fixed conflicts.
CI: https://ci.nodejs.org/job/node-test-pull-request/63804/
CI: https://ci.nodejs.org/job/node-test-pull-request/63811/ 💛
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()https://github.com/nodejs/node/actions/runs/12280911806PR-URL: https://github.com/nodejs/node/pull/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: https://github.com/nodejs/node/pull/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: https://github.com/nodejs/node/pull/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-squashlabel to land the PR as one commit, orcommit-queue-rebaseto land as separate commits.
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 55413004c8ab489a03793b80c20d2ec6552668c0