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

doc: add steam pipelining note on Http usage #41796

Merged

Conversation

RafaelGSS
Copy link
Member

Address: #26311

Reference: #41137

cc/@ronag @mcollina

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jan 31, 2022
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
@RafaelGSS RafaelGSS force-pushed the doc/add-stream-pipeline-advice branch 2 times, most recently from fb8c5a9 to 7f235dc Compare January 31, 2022 20:30
@ronag ronag requested a review from mcollina January 31, 2022 20:35
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

Copy link
Contributor

@mmarchini mmarchini 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

@RafaelGSS RafaelGSS force-pushed the doc/add-stream-pipeline-advice branch from 7f235dc to fd7932d Compare February 1, 2022 12:35
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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@@ -2517,6 +2517,28 @@ run().catch(console.error);
after the `callback` has been invoked. In the case of reuse of streams after
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
Member Author

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

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2022
@nodejs-github-bot nodejs-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:master Feb 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]>
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants