Skip to content

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

Closed
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:finished-outgoing
Closed

stream: finished waits for 'close' on OutgoingMessage#36648
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:finished-outgoing

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
mcollina
mcollina reviewed Dec 27, 2020
View reviewed changes
ronag
ronag commented Dec 27, 2020
View reviewed changes
@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

nodejs-github-bot commented Jan 5, 2021

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 force-pushed the finished-outgoing branch from ac36161 to fef3c07 Compare January 5, 2021 21:30
@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

nodejs-github-bot commented Jan 5, 2021

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

nodejs-github-bot commented Jan 6, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 6, 2021

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

nodejs-github-bot commented Jan 7, 2021

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

nodejs-github-bot commented Jan 9, 2021

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

nodejs-github-bot commented Jan 9, 2021

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

@mcollina mcollina mcollina approved these changes

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