Skip to content

Conversation

@RafaelGSS
Copy link
Member

Address: #26311

Reference: #41137

cc/@ronag@mcollina

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jan 31, 2022
@RafaelGSSRafaelGSSforce-pushed the doc/add-stream-pipeline-advice branch 2 times, most recently from fb8c5a9 to 7f235dcCompareJanuary 31, 2022 20:30
@ronagronag requested a review from mcollinaJanuary 31, 2022 20:35
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@mmarchinimmarchini left a comment

Choose a reason for hiding this comment

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

nit: replace steam with stream in commit message

@RafaelGSSRafaelGSSforce-pushed the doc/add-stream-pipeline-advice branch from 7f235dc to fd7932dCompareFebruary 1, 2022 12:35
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

failure, this can cause event listener leaks and swallowed errors.

`stream.pipeline()` closes all the streams when an error is raised.
The `IncomingRequest` usage with `pipeline` could lead to an unexpected behavior
Copy link
Member

Choose a reason for hiding this comment

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

What is IncomingRequest? Did you mean IncomingMessage? In the example below res is a ServerResponse or OutgoingMessage.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Actually, I meant an Incoming Request in general. The idea is to show that a socket/request usage with pipeline could lead to a error

@mcollinamcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2022
@nodejs-github-botnodejs-github-bot merged commit 1c7a74e into nodejs:masterFeb 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 1c7a74e

ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
PR-URL: #41796 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
@ruyadornoruyadorno mentioned this pull request Feb 8, 2022
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
PR-URL: #41796 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
PR-URL: #41796 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
PR-URL: nodejs#41796 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
PR-URL: nodejs#41796 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 8, 2022
PR-URL: #41796 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
PR-URL: #41796 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@RafaelGSS@nodejs-github-bot@mcollina@jasnell@lpinca@ronag@mmarchini@Mesteery