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

http: fix deferToConnect comments #27876

Closed
wants to merge 1 commit into from
Closed

http: fix deferToConnect comments #27876

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented May 25, 2019 •
edited
Loading

Fixes #27875. Fix description to actual intended and current behaviour.

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

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. labels May 25, 2019
@Trott
Copy link
Member

Trott commented May 26, 2019

Is it non-trivial to write a reliable test for this?

@ronag
Copy link
Member Author

ronag commented May 26, 2019

@Trott Before I spend more time on this I'd like to know what the intended behavior here is? writable or connected? I actually think the current behavior of just waiting for writable might be the intended behavior... in which case just the comment needs updating. ping @mcollina

@mcollina
Copy link
Member

I do not see what is the bug that you are trying to fix. Essentially, I do not know what behavior you would like to change. Neither this PR or #27875 contain an understandable example.

@ronag
Copy link
Member Author

ronag commented May 26, 2019

I can't explain it any better than stated in the issue and my previous comment.

@ronag
Copy link
Member Author

ronag commented May 26, 2019 •
edited
Loading

I've updated the PR. I believe the current behavior is as intended and it's just the description that needs a slight modification. The method could also possibly have a better name but I dare not modify it...

mcollina
mcollina reviewed May 26, 2019
View reviewed changes
lib/_http_client.js Outdated
@@ -705,7 +705,7 @@ function onSocketNT(req, socket) {
ClientRequest.prototype._deferToConnect = _deferToConnect;
function _deferToConnect(method, arguments_, cb) {
// This function is for calls that need to happen once the socket is
// connected and writable. It's an important promisy thing for all the socket
// assigned and writable. It's an important promisy thing for all the socket
Copy link
Member

Choose a reason for hiding this comment

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

You might want to expand this a bit more. What does it mean for the socket to be assigned?

Copy link
Member Author

Choose a reason for hiding this comment

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

this.socket != null?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but why? Sockets are assigned to a request when they are available in the agent.

Copy link
Member Author

Choose a reason for hiding this comment

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

expanded to follow the description from https://nodejs.org/api/http.html#http_event_socket

mcollina
mcollina approved these changes May 26, 2019
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, good work!

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@BridgeAR BridgeAR changed the title Fix defer to connect http: fix deferToConnect comments May 29, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Jun 2, 2019

Landed in c6f545a

@Trott Trott closed this Jun 2, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 2, 2019
@ronag @Trott
http: fix deferToConnect comments
c6f545a 
PR-URL: nodejs#27876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 3, 2019
@ronag @targos
http: fix deferToConnect comments
d948656 
PR-URL: #27876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jun 3, 2019
Merged
@lpinca lpinca mentioned this pull request Jun 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

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. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ClientRequest._deferToConnect doesn't wait until connected
4 participants
@ronag @Trott @mcollina @nodejs-github-bot