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.

Sign up for GitHub

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

Jump to bottom

stream: finished waits for 'close' on OutgoingMessage #36648

Closed
wants to merge 2 commits into from
Closed

stream: finished waits for 'close' on OutgoingMessage #36648

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 27, 2020 •
edited
Loading

Don't invoke finished callback until OutgoingMessage has
emitted 'close'.

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 requested a review from mcollina December 27, 2020 20:31
@ronag
Copy link
Member Author

ronag commented Dec 27, 2020

@nodejs/http

@ronag ronag added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. labels Dec 27, 2020
@ronag ronag requested a review from mcollina December 27, 2020 22:23
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2021
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@ronag
Copy link
Member Author

ronag commented Jan 5, 2021

@nodejs/http @nodejs/streams this needs review

@ronag
stream: finished waits for 'close' on OutgoingMessage
b9d1dda 
Don't invoke finished callback until OutgoingMessagehas
emitted 'close'.
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35310/

mcollina
mcollina approved these changes Jan 5, 2021
View reviewed changes
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

@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 5, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35314/

@mmarchini mmarchini mentioned this pull request Jan 6, 2021
Closed
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35324/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35330/

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 7, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35344/

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 9, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35391/

mcollina
mcollina approved these changes Jan 9, 2021
View reviewed changes
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

CI: https://ci.nodejs.org/job/node-test-pull-request/35408/

@jasnell
Copy link
Member

jasnell commented Jan 11, 2021

Landed in 52546c0

@jasnell jasnell closed this Jan 11, 2021
jasnell pushed a commit that referenced this pull request Jan 11, 2021
@ronag @jasnell
stream: finished waits for 'close' on OutgoingMessage
52546c0 
Don't invoke finished callback until OutgoingMessagehas
emitted 'close'.

PR-URL: #36648
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@ronag @danielleadams
stream: finished waits for 'close' on OutgoingMessage
abae61e 
Don't invoke finished callback until OutgoingMessagehas
emitted 'close'.

PR-URL: #36648
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
Merged
@targos
Copy link
Member

targos commented May 25, 2021

The test fails if I cherry-pick this to v14.x-staging. Assuming it shouldn't land on that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@mafintosh mafintosh Awaiting requested review from mafintosh

@addaleax addaleax Awaiting requested review from addaleax

@lpinca lpinca Awaiting requested review from lpinca

@lundibundi lundibundi Awaiting requested review from lundibundi

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ronag @nodejs-github-bot @jasnell @targos @mcollina