Skip to content

doc: add steam pipelining note on Http usage - #41796

Merged
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
RafaelGSS:doc/add-stream-pipeline-advice
Feb 6, 2022
Merged

doc: add steam pipelining note on Http usage#41796
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
RafaelGSS:doc/add-stream-pipeline-advice

Conversation

@RafaelGSS

Copy link
Copy Markdown
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
Comment threaddoc/api/stream.md Outdated
Comment threaddoc/api/stream.md Outdated
@RafaelGSS
RafaelGSSforce-pushed the doc/add-stream-pipeline-advice branch 2 times, most recently from fb8c5a9 to 7f235dcCompareJanuary 31, 2022 20:30
@ronag
ronag requested a review from mcollinaJanuary 31, 2022 20:35

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mmarchinimmarchini left a comment

Copy link
Copy Markdown
Contributor

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

@RafaelGSS
RafaelGSSforce-pushed the doc/add-stream-pipeline-advice branch from 7f235dc to fd7932dCompareFebruary 1, 2022 12:35

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threaddoc/api/stream.md
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
Copy Markdown
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
Copy Markdown
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-bot
nodejs-github-bot merged commit 1c7a74e into nodejs:masterFeb 6, 2022
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 1c7a74e

ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
PR-URL: #41796
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
PR-URL: #41796
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
PR-URL: nodejs#41796
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 8, 2022
PR-URL: #41796
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
PR-URL: #41796
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
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