GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: adjust src hwm when pipelining by ronag · Pull Request #40751 · nodejs/node

Conversation

@ronag

@ronag ronag commented Nov 7, 2021

Copy link
Copy Markdown
Member

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ronag
ronag requested a review from mcollina November 7, 2021 09:50
@ronag

ronag commented Nov 7, 2021

Copy link
Copy Markdown
Member Author

@nodejs/startup @addaleax
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag added the stream Issues and PRs related to Node.js streams. 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
@ronag
ronag force-pushed the pipeline-hwm branch from dbe4a83 to 46f9638 Compare November 7, 2021 09:51
mcollina
mcollina requested changes Nov 7, 2021
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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');

@mcollina mcollina Nov 7, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ronag ronag Nov 7, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/streams/pipeline.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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 whose changes need more time before an LTS backport. 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 force-pushed the pipeline-hwm branch from a97bff0 to 67fc5f6 Compare November 10, 2021 09:55
@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. Only starts once the PR has an approving review. label Nov 10, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 10, 2021
@nodejs-github-bot

nodejs-github-bot commented Nov 10, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag removed the needs-ci PRs that need a full CI run. label Nov 10, 2021
@ronag
ronag force-pushed the pipeline-hwm branch from 37482b0 to 16a2fb6 Compare November 10, 2021 15:17
@ronag

ronag commented Nov 10, 2021

Copy link
Copy Markdown
Member Author

@nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag requested a review from lpinca November 10, 2021 15:22
mcollina
mcollina approved these changes Nov 10, 2021
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ronag ronag added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 10, 2021
@lpinca

lpinca commented Nov 10, 2021

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Nov 10, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Nov 11, 2021
CI Reliability 2021-11-11 nodejs/reliability#114
Open
31 tasks
@mcollina

mcollina commented Nov 11, 2021

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca

lpinca commented Nov 11, 2021

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Nov 11, 2021

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Nov 11, 2021

Copy link
Copy Markdown
Member Author

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.
mcollina reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Nov 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag force-pushed the pipeline-hwm branch from 16a2fb6 to 2d35f16 Compare November 19, 2021 18:21
@nodejs-github-bot

nodejs-github-bot commented Nov 19, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Nov 19, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag removed the needs-citgm PRs that need a CITGM CI run. label Nov 19, 2021
This was referenced Nov 20, 2021
CI Reliability 2021-11-20 nodejs/reliability#123
Open
CI Reliability 2021-11-21 nodejs/reliability#124
Open
CI Reliability 2021-11-22 nodejs/reliability#125
Open
@ronag
ronag force-pushed the pipeline-hwm branch from 2d35f16 to 77eeaa1 Compare November 22, 2021 08:44
@nodejs-github-bot

nodejs-github-bot commented Nov 22, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
stream: adjust src hwm when pipelining
662d3e8
@ronag
ronag force-pushed the pipeline-hwm branch from 7f09855 to 662d3e8 Compare November 22, 2021 12:28
ronag added 3 commits November 22, 2021 13:31
@ronag
fixup
821ab1a
@ronag
fixup
541817e
@ronag
ifxup
4f3d998
This was referenced Nov 23, 2021
CI Reliability 2021-11-23 nodejs/reliability#126
Open
CI Reliability 2021-11-24 nodejs/reliability#127
Open
CI Reliability 2021-11-25 nodejs/reliability#128
Open
CI Reliability 2021-11-26 nodejs/reliability#129
Open
@nodejs-github-bot

nodejs-github-bot commented Nov 27, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag requested review from jasnell and mcollina November 27, 2021 17:18
This was referenced Nov 28, 2021
CI Reliability 2021-11-28 nodejs/reliability#131
Open
CI Reliability 2021-11-29 nodejs/reliability#132
Open
@aduh95

aduh95 commented Apr 3, 2022

Copy link
Copy Markdown
Contributor

This needs a rebase.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95

aduh95 commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@avivkeller avivkeller closed this Aug 9, 2026
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

Uh oh!

There was an error while loading. Please reload this page.

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. stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@ronag @nodejs-github-bot @lpinca @mcollina @addaleax @aduh95 @jasnell @avivkeller
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.