stream: allow readable to end early without error by ronag · Pull Request #40881 · nodejs/node

ronag · 2021-11-19T13:03:25Z

No description provided.

ronag · 2021-11-19T13:03:35Z

@nodejs/streams

ronag · 2021-11-19T13:16:19Z

@benjamingr I think this PR will be necessary for e.g. take(n).

nodejs-github-bot · 2021-11-19T13:19:15Z

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

nodejs-github-bot · 2021-11-19T17:47:46Z

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

ronag · 2021-11-22T08:32:04Z

lib/internal/streams/pipeline.js

-
+      } else if (isIterable(ret)) {
        finishCount++;
        pump(ret, stream, finish);


pump has its own cleanup logic through async iterator.

ronag · 2021-11-25T20:03:25Z

@nodejs/streams I noticed some other issues while working on this. Please see updated tests.

lib/internal/streams/pipeline.js

nodejs-github-bot · 2021-12-04T20:39:37Z

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

lpinca · 2021-12-04T21:44:54Z

test/parallel/test-stream-pipeline.js

+    return ret;
+  }, common.mustCall((err, val) => {
+    assert.strictEqual(err, undefined);
+    assert.strictEqual(val, 'helloworld');


This test seems to be broken. val is undefined, I'm not sure why the thrown exception does not make the process exit. I did not investigate.

Yea.. that's weird. Fixed the test.

Updated test looks good, but why the process did not exit?

I can reproduce the same issue with current Node.js version (v17.2.0) so it is not related to this PR but it is something to investigate and fix.

It seems that the error thrown in the callback is caught and swallowed here

node/lib/internal/streams/pipeline.js

Lines 163 to 164 in 147d23b

} catch (err) {

finish(error !== err ? aggregateTwoErrors(error, err) : err);

.

Fixed, PTAL, f0ec29b

nodejs-github-bot · 2021-12-05T11:36:09Z

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

nodejs-github-bot · 2021-12-05T16:22:48Z

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

lpinca

Can you please add a test for cf85105?

nodejs-github-bot · 2021-12-06T15:57:11Z

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

ronag · 2021-12-06T19:42:29Z

Landed in 1fa507f

PR-URL: #40881 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

danielleadams · 2022-01-31T01:49:44Z

@ronag do you mind making a backport PR for this for v16.x-staging? It did not land cleanly when pulling into the 16.x release.

PR-URL: nodejs#40881 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

ronag requested a review from mcollina November 19, 2021 13:03

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 19, 2021

ronag added request-ci Add this label to start a Jenkins CI on a PR. stream Issues and PRs related to the stream subsystem. and removed needs-ci PRs that need a full CI run. labels Nov 19, 2021

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

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 19, 2021

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

github-actions bot mentioned this pull request Nov 20, 2021

CI Reliability 2021-11-20 nodejs/reliability#123

Open

47 tasks

jasnell approved these changes Nov 20, 2021

View reviewed changes

github-actions bot mentioned this pull request Nov 21, 2021

CI Reliability 2021-11-21 nodejs/reliability#124

Open

41 tasks

ronag requested a review from jasnell November 21, 2021 17:21

github-actions bot mentioned this pull request Nov 22, 2021

CI Reliability 2021-11-22 nodejs/reliability#125

Open

40 tasks

ronag requested a review from lpinca November 22, 2021 07:57

ronag mentioned this pull request Nov 22, 2021

stream: don't emit finish after destroy #40852

Closed

ronag commented Nov 22, 2021

View reviewed changes

ronag force-pushed the readable-take branch from 6288e57 to 308e4a2 Compare November 22, 2021 15:10

github-actions bot mentioned this pull request Nov 23, 2021

CI Reliability 2021-11-23 nodejs/reliability#126

Open

45 tasks

ronag marked this pull request as draft November 23, 2021 11:37

This was referenced Nov 24, 2021

CI Reliability 2021-11-24 nodejs/reliability#127

Open

CI Reliability 2021-11-25 nodejs/reliability#128

Open

ronag force-pushed the readable-take branch from 917fb2a to 59e377c Compare November 25, 2021 20:00

ronag marked this pull request as ready for review November 25, 2021 20:02

wa-Nadoo reviewed Nov 25, 2021

View reviewed changes

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

ronag commented Nov 26, 2021

View reviewed changes

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

lpinca requested changes Dec 4, 2021

View reviewed changes

fixup

f0ec29b

ronag requested a review from lpinca December 4, 2021 22:13

github-actions bot mentioned this pull request Dec 5, 2021

CI Reliability 2021-12-05 nodejs/reliability#135

Open

20 tasks

fixup: nextTick callback

cf85105

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2021

lpinca approved these changes Dec 5, 2021

View reviewed changes

github-actions bot mentioned this pull request Dec 6, 2021

CI Reliability 2021-12-06 nodejs/reliability#136

Open

21 tasks

ronag added a commit that referenced this pull request Dec 6, 2021

stream: allow readable to end early without error

1fa507f

PR-URL: #40881 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

ronag closed this Dec 6, 2021

danielleadams mentioned this pull request Dec 14, 2021

v17.3.0 release proposal #41167

Merged

danielleadams added the backport-requested-v16.x label Jan 31, 2022

juanarbol mentioned this pull request May 29, 2022

stream: allow returning null from pipeline tail #42078

Merged

targos added dont-land-on-v16.x and removed backport-requested-v16.x labels Jul 31, 2022

	} catch (err) {
	finish(error !== err ? aggregateTwoErrors(error, err) : err);

Uh oh!

Conversation

ronag commented Nov 19, 2021

Uh oh!

ronag commented Nov 19, 2021

Uh oh!

ronag commented Nov 19, 2021

Uh oh!

nodejs-github-bot commented Nov 19, 2021

Uh oh!

nodejs-github-bot commented Nov 19, 2021

Uh oh!

ronag Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag commented Nov 25, 2021

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 4, 2021

Uh oh!

lpinca Dec 4, 2021

Choose a reason for hiding this comment

Uh oh!

ronag Dec 4, 2021

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 5, 2021

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 5, 2021

Choose a reason for hiding this comment

Uh oh!

lpinca Dec 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag Dec 5, 2021

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 5, 2021

Uh oh!

nodejs-github-bot commented Dec 5, 2021

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 6, 2021 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Dec 6, 2021

Uh oh!

danielleadams commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ronag Nov 22, 2021 •

edited

Loading

lpinca Dec 5, 2021 •

edited

Loading

nodejs-github-bot commented Dec 6, 2021 •

edited by ronag

Loading