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.

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

stream: avoid destroying writable source #32198

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 11, 2020

User might still want to be able to use the writable side
of src. This is in the case where e.g. the Duplex input
is not directly connected to its output. Such a case could
happen when the Duplex is reading from a socket and then echos
the data back on the same socket.

Fixes: 4d93e10#commitcomment-37751035

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Mar 11, 2020
@ronag ronag requested a review from jasnell March 11, 2020 12:57
ronag referenced this pull request Mar 11, 2020
If the last stream in a pipeline is still usable/readable
don't destroy it to allow further composition.

Fixes: #32105

PR-URL: #32110
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

@MylesBorins
Copy link
Contributor

can we fast track this? I'd like to see it land in the upcoming 13.11.0 release

@ronag looks like this needs a rebase

@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 11, 2020
@ronag
Copy link
Member Author

ronag commented Mar 11, 2020

rebased @MylesBorins

@nodejs-github-bot
Copy link
Collaborator

const src = new PassThrough();
assert.strictEqual(src.writable, true);
const dst = new PassThrough();
pipeline(src, dst, common.mustCall((err) => {
Copy link
Member

Choose a reason for hiding this comment

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

A test that has a Duplex piping back in to itself would be good also

Copy link
Member Author

@ronag ronag Mar 11, 2020

Choose a reason for hiding this comment

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

I suppose this is a nit? Since we might want to fast-track this would you mind if I do so in a follow up PR later?

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 11, 2020
@ronag ronag requested review from mcollina and lpinca March 11, 2020 16:53
@MylesBorins
Copy link
Contributor

@ronag fwiw you can't approve fastrack on your own PR

@ronag
Copy link
Member Author

ronag commented Mar 11, 2020

@ronag fwiw you can't approve fastrack on your own PR

Np, I was +1 the rebase part of your comment. I'll remove my +1.

@jasnell
Copy link
Member

jasnell commented Mar 11, 2020

+1 to fast tracking

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 and +1 to fast track

@nodejs-github-bot
Copy link
Collaborator

@MylesBorins
Copy link
Contributor

@ronag FWIW this patch is not going to land cleanly on v13.x. You might want to get started on a backport now if you have time. very much trying to get this out in today's release.

@nodejs-github-bot
Copy link
Collaborator

User might still want to be able to use the writable side
of src. This is in the case where e.g. the Duplex input
is not directly connected to its output. Such a case could
happen when the Duplex is reading from a socket and then echos
the data back on the same socket.

Fixes: nodejs@4d93e10#commitcomment-37751035
@ronag
Copy link
Member Author

ronag commented Mar 11, 2020

Fixed conflicts

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 11, 2020

@nodejs-github-bot
Copy link
Collaborator

MylesBorins pushed a commit that referenced this pull request Mar 12, 2020
User might still want to be able to use the writable side
of src. This is in the case where e.g. the Duplex input
is not directly connected to its output. Such a case could
happen when the Duplex is reading from a socket and then echos
the data back on the same socket.

PR-URL: #32198
Refs: 4d93e10#commitcomment-37751035
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 2bfb340

MylesBorins pushed a commit that referenced this pull request Mar 12, 2020
User might still want to be able to use the writable side
of src. This is in the case where e.g. the Duplex input
is not directly connected to its output. Such a case could
happen when the Duplex is reading from a socket and then echos
the data back on the same socket.

Backport-PR-URL: #32212
PR-URL: #32198
Refs: 4d93e10#commitcomment-37751035
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants