Skip to content

doc: fix wrong response.end() at request.socket #19507

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
ikasumiwt wants to merge 1 commit into from
Closed

doc: fix wrong response.end() at request.socket #19507

ikasumiwt wants to merge 1 commit into from
+2 −3

Conversation

@ikasumiwt
Copy link
Contributor

@ikasumiwt ikasumiwt commented Mar 21, 2018 •
edited
Loading

Fixed response.end() to request.end() at request.socket because request.socket property was null when called after request.end() method.

Checklist

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Mar 21, 2018
lpinca
lpinca reviewed Mar 21, 2018
View reviewed changes
doc/api/http.md Outdated
Copy link
Member

@lpinca lpinca Mar 21, 2018

Choose a reason for hiding this comment

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

Both seem incorrect. It's null when ClientRequest is instantiated via http.request() because a socket has not been assigned yet but it seems the property is never nulled after the 'socket' event.

Copy link
Member

@mcollina mcollina Apr 10, 2018

Choose a reason for hiding this comment

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

I agree with @lpinca. It also seems something we might want to do (nulling the socket) not after request.end() but rather after the response has been fully consumed.

Copy link
Member

@jasnell jasnell Apr 10, 2018

Choose a reason for hiding this comment

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

hmmm... actually good point. Missed that on my initial read through

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

@nodejs/http @nodejs/http2 @nodejs/streams PTAL

jasnell
jasnell previously approved these changes Apr 9, 2018
View reviewed changes
lpinca
lpinca requested changes Apr 9, 2018
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

Making my -1 explicit to prevent this from being landed as this change doesn't seem correct.

@shigeki
Copy link
Contributor

shigeki commented Apr 9, 2018

Removing the sentence of response.end() seems to get a right correction.
@ikasumiwt Can you change this?

@jasnell jasnell dismissed their stale review April 10, 2018 15:37

agreeing with mcollina and lpinca

@ikasumiwt ikasumiwt force-pushed the feature/fix-request-socket-doc branch from 9c42196 to d9ce804 Compare April 12, 2018 16:32
@ikasumiwt
Copy link
Contributor Author

ikasumiwt commented Apr 12, 2018

Thanks for reviewing, and sorry for reply late.

My change was wrong, so I changed to remove response.end() sentence.
Could you please re-review this PR?

@lpinca
Copy link
Member

lpinca commented Apr 12, 2018

@ikasumiwt thanks. LGTM.

@lpinca
Copy link
Member

lpinca commented Apr 12, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/504/

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018
shigeki
shigeki approved these changes Apr 13, 2018
View reviewed changes
Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

Thanks. Good Job.

mcollina
mcollina approved these changes Apr 13, 2018
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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 13, 2018
@ikasumiwt @BridgeAR
doc: fix wrong response.end() at request.socket
019a2c4 
PR-URL: nodejs#19507
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

BridgeAR commented Apr 13, 2018

Landed in 019a2c4 🎉

@BridgeAR BridgeAR closed this Apr 13, 2018
jasnell pushed a commit that referenced this pull request Apr 16, 2018
@ikasumiwt @jasnell
doc: fix wrong response.end() at request.socket
fbf9e06 
PR-URL: #19507
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
@ikasumiwt @BridgeAR
doc: fix wrong response.end() at request.socket
993b96b 
PR-URL: nodejs#19507
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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

@lpinca lpinca lpinca approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

+1 more reviewer

@shigeki shigeki shigeki approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ikasumiwt @BridgeAR @shigeki @lpinca @mcollina @jasnell @trivikr @nodejs-github-bot