Skip to content

http: set IncomingMessage.destroyed#33131

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
Closed
ronag wants to merge 2 commits into from
Closed

http: set IncomingMessage.destroyed #33131

ronag wants to merge 2 commits into from
+94 −4

Conversation

@ronag
Copy link
Member

@ronagronag commented Apr 28, 2020

IncomingMessage is a Readable stream and should properly
set the destroyed property.

This code is a little weird in general but this is the best solution I can think of until a bigger refactoring.

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
http: set IncomingMessage.destroyed
cea30bb
IncomingMessage is a Readable stream and should properly set the destroyed property.
@ronagronag added the http Issues or PRs related to the http subsystem. label Apr 28, 2020
@ronagronag requested a review from mcollinaApril 28, 2020 17:19
@ronag
Copy link
MemberAuthor

ronag commented Apr 28, 2020

@nodejs/http @nodejs/web-server-frameworks

@ronagronag mentioned this pull request Apr 28, 2020
Open
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 28, 2020•
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31063/ (💛)

mcollina
mcollina approved these changes Apr 28, 2020
View reviewed changes
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

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 29, 2020
juanarbol
juanarbol approved these changes Apr 29, 2020
View reviewed changes
Copy link
Member

@juanarboljuanarbol left a comment

Choose a reason for hiding this comment

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

LGMT

@ronagronag mentioned this pull request Apr 29, 2020
Open
@ronagronag added wip Issues and PRs that are still a work in progress. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 30, 2020
@ronag
Copy link
MemberAuthor

ronag commented Apr 30, 2020•
edited
Loading

I've found more cases that need to be fixed. Especially on the server side.

Refs for final commit msg:
#33147
#33118

@ronagronagforce-pushed the http-outgoing-destroyed branch from 7c5696c to 39b5d2bCompareApril 30, 2020 08:03
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 30, 2020•
edited by ronag
Loading

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

@ronagronag removed the wip Issues and PRs that are still a work in progress. label Apr 30, 2020
@ronag
Copy link
MemberAuthor

ronag commented Apr 30, 2020

@nodejs/http: I missed a few cases earlier. PTAL.

@ronagronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 1, 2020
@addaleax
Copy link
Member

addaleax commented May 9, 2020

Landed in 28e6626

@addaleaxaddaleax closed this May 9, 2020
addaleax pushed a commit that referenced this pull request May 9, 2020
@ronag@addaleax
http: set IncomingMessage.destroyed
28e6626
IncomingMessage is a Readable stream and should properly set the destroyed property. PR-URL: #33131 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@ronagronag mentioned this pull request May 10, 2020
Closed
codebytere pushed a commit that referenced this pull request May 11, 2020
@ronag@codebytere
http: set IncomingMessage.destroyed
b634d4b
IncomingMessage is a Readable stream and should properly set the destroyed property. PR-URL: #33131 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
@codebyterecodebytere mentioned this pull request May 18, 2020
Merged
This was referenced May 27, 2020
Closed
Closed
@ronagronag mentioned this pull request May 29, 2020
Open
@mcollinamcollina mentioned this pull request Jun 1, 2020
Closed
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juanarboljuanarboljuanarbol approved these changes

@addaleaxaddaleaxAwaiting requested review from addaleax

@mcollinamcollinaAwaiting requested review from mcollina

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.httpIssues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ronag@nodejs-github-bot@addaleax@mcollina@juanarbol@BridgeAR