Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: adjust src hwm when pipelining #40751

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

stream: adjust src hwm when pipelining #40751

wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 7, 2021

No description provided.

@ronag ronag requested a review from mcollina November 7, 2021 09:50
@ronag
Copy link
Member Author

ronag commented Nov 7, 2021

@nodejs/startup @addaleax

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Nov 7, 2021
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 7, 2021
mcollina
mcollina requested changes Nov 7, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A doc change is needed. This also change/simplify the pipe logic and I like it.

I'm not sure I would backport it immediately to LTS, but I'm not sure it's strictly semver-major either.

test/parallel/test-stream-pipeline.js
@@ -1350,7 +1347,7 @@ const tsp = require('timers/promises');
});
const cb = common.mustCall((err) => {
assert.strictEqual(err.name, 'AbortError');
assert.strictEqual(res, '012345');
assert.strictEqual(res, '01234');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a timing difference due to how streams are resumed with 'readable' event.

lib/internal/streams/pipeline.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the pipeline-hwm branch 3 times, most recently from 4d68da2 to 3d0d042 Compare November 8, 2021 05:53
@mcollina mcollina added baking-for-lts PRs that need to wait before landing in a LTS release. semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Nov 8, 2021
@ronag ronag requested a review from mcollina November 10, 2021 15:10
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag removed the needs-ci PRs that need a full CI run. label Nov 10, 2021
@ronag
Copy link
Member Author

ronag commented Nov 10, 2021

@nodejs/streams

@ronag ronag requested a review from lpinca November 10, 2021 15:22
mcollina
mcollina approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 10, 2021
@lpinca
Copy link
Member

lpinca commented Nov 10, 2021

What happens if the source is already piped to another destination via source.pipe(destination)? Does this allow the same source to be piped to multiple destinations?

@ronag
Copy link
Member Author

ronag commented Nov 10, 2021 •
edited
Loading

Combining pipe and read is not a good idea... so no

@github-actions github-actions bot mentioned this pull request Nov 11, 2021
Open
31 tasks
@mcollina
Copy link
Member

What happens if the source is already piped to another destination via source.pipe(destination)? Does this allow the same source to be piped to multiple destinations?

It will work but the consumption will be driven by pipeline as the 'readable' takes precedence.

@ronag could you add a test for this case?

@lpinca
Copy link
Member

lpinca commented Nov 11, 2021

It will work but the consumption will be driven by pipeline as the 'readable' takes precedence.

If my understanding is correct, then consumption will be driven by the fastest destination, not the slowest, potentially messing up backpressure handling, right?

@mcollina
Copy link
Member

If my understanding is correct, then consumption will be driven by the fastest destination, not the slowest, potentially messing up backpressure handling, right?

No, not really. The same happens if you mix async iterators and .pipe(). AsyncIterators use .read() and they drive the consumption of the source data.

Unfortuunately this is the only way .read() can work reliably. See #18994 and #18058.

@ronag
Copy link
Member Author

ronag commented Nov 11, 2021

What happens if the source is already piped to another destination via source.pipe(destination)? Does this allow the same source to be piped to multiple destinations?

It will work but the consumption will be driven by pipeline as the 'readable' takes precedence.

@ronag could you add a test for this case?

I don't really see what a test here contributes? It uses an existing api.

@addaleax
Copy link
Member

Fwiw, I marked this semver-major because it’s a big breaking change to stop using .pipe() here – my understanding is that this breaks piping to multiple destinations completely, and even if not, this is missing pipe/unpipe events, it’s breaking manual src.emit('data') calls, etc. (not saying that people should rely on this – but reality is that they do).

either we make pipeline use read or make async gen use pipe

The latter sounds a lot safer to me.

@addaleax addaleax added needs-citgm PRs that need a CITGM CI run. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 18, 2021
@ronag
Copy link
Member Author

ronag commented Nov 18, 2021 •
edited
Loading

IMO if we merge this we are moving towards some from of deprecation of pipe.

If we don’t merge this we’re in this inconsistent state where sometimes we use pipe and sometimes not and the situation is quite unpredictable for our users.

I think we have to choose which api we recommend and stick with it in core at least.

readable is simpler
pipe has better compat and maybe more feature rich in terms of multiple consumers?

@mcollina wdyt?

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Nov 19, 2021

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2793/

@ronag ronag removed the needs-citgm PRs that need a CITGM CI run. label Nov 19, 2021
This was referenced Nov 20, 2021
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Nov 23, 2021
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag requested review from mcollina and jasnell November 27, 2021 17:18
This was referenced Nov 28, 2021
Open
Open
@aduh95
Copy link
Contributor

aduh95 commented Apr 3, 2022

This needs a rebase.

@aduh95
Copy link
Contributor

aduh95 commented Sep 8, 2022

This needs a rebase, if we want to still merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca Awaiting requested review from lpinca

@mcollina mcollina Awaiting requested review from mcollina

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronag @nodejs-github-bot @lpinca @mcollina @addaleax @aduh95 @jasnell