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.

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

http: DRY ClientRequest.prototype._deferToConnect #2769

Closed
wants to merge 3 commits into from

Conversation

ahoym
Copy link
Contributor

@ahoym ahoym commented Sep 9, 2015

Came across some repetitive lines when reviewing code.

Logic for calling the passed in socket method and/or cb was duplicated. Extracted these calls into a separate function, callSocketMethod, and invoked this new function in place of the replaced code.

Logic for calling the passed in socket method and/or callback was duplicated.
@@ -504,19 +504,17 @@ ClientRequest.prototype._deferToConnect = function(method, arguments_, cb) {
// in the future (when a socket gets assigned out of the pool and is
// eventually writable).
var self = this;
var callSocketMethod = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this function callSocketMethod()

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Sep 9, 2015
@ahoym
Copy link
Contributor Author

ahoym commented Sep 9, 2015

Comments have been addressed. Of course, any more are welcome.

@kahnvex
Copy link

kahnvex commented Sep 9, 2015

🍇

@@ -504,19 +504,17 @@ ClientRequest.prototype._deferToConnect = function(method, arguments_, cb) {
// in the future (when a socket gets assigned out of the pool and is
// eventually writable).
var self = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, self doesn't seem to be necessary. Mind removing it? It can be replaced with, as far as I can tell, a var socket = this.socket. The bottom reference to self can just be used as this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I converted the code as you suggested (good catch btw), and that caused the error

cannot read property 'writable' of null

if (socket.writable) { // socket used to be self.socket
  ...

This seems to stem from the initial _deferToConnect call, where this.socket is undefined. When the ClientRequest catches the 'socket' event, the socket variable in onSocket() refers to the initial undefined value which was scoped in. As a result, everything blows up when trying to read the writable property of that.

A work around that I found was to have onSocket() accept a sock argument (given by the socket event), and update socket with that parameter. So this piece of code works:

ClientRequest.prototype._deferToConnect = function(method, arguments_, cb) {
  // ...
  var socket = this.socket;
  function callSocketMethod() {
    if (method)
      socket[method].apply(socket, arguments_);
    if (typeof cb === 'function')
      cb();
  }
  var onSocket = function(sock) { // Now accepts the socket from the 'socket' event.
    socket = sock || socket; // Update socket reference.
    if (socket.writable) {
      callSocketMethod();
    } else {
      socket.once('connect', callSocketMethod);
    }
  };
  if (!socket) {
    this.once('socket', onSocket);
  } else {
    onSocket();
  }
};

However I'm not completely sure on the repercussions of this. Maybe leaving the existing implementation would be for the better? I'm interested in what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, you're right, that is something I missed originally. I'd say let's keep the current implementation, as that new one wouldn't really improve the quality of the code. Sorry about the run around!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, then I'll leave this PR as it is. And no need to apologize, we got some documentation and a deeper understanding of the code here 😄

@brendanashworth
Copy link
Contributor

@ahoym
Copy link
Contributor Author

ahoym commented Sep 14, 2015

Seems like the build failed?

Looks like only the node-test-commit-arm (#603) job failed.
Digging deeper, the pi1-raspbian-wheezy-1_of_2 and pi2-raspbian-wheezy-2_of_2 builds are both erroring out with this:

returned status code 128:
stdout: 
stderr: error: object directory /Users/rvagg/git/iojs/io.js/.git/objects does not exist; check .git/objects/info/alternates.
error: refs/remotes/origin/archived-io.js-v0.10 does not point to a valid object!
error: refs/remotes/origin/archived-io.js-v0.12 does not point to a valid object!
error: refs/remotes/origin/mergev010/base does not point to a valid object!
...

@rvagg
Copy link
Member

rvagg commented Sep 15, 2015

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

those arm errors were fixed yesterday

@brendanashworth
Copy link
Contributor

CI is happy, this LGTM. @cjihrig ?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 16, 2015

Yep, LGTM

@ahoym
Copy link
Contributor Author

ahoym commented Sep 21, 2015

Bump

cjihrig pushed a commit that referenced this pull request Sep 22, 2015
Logic for calling the passed in socket method and/or callback
was duplicated. This commit refactors the relevant code to
remove the redundancy.

PR-URL: #2769
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brendan Ashworth <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Sep 22, 2015

Thanks! Landed in 79d2c4e after squashing.

@cjihrig cjihrig closed this Sep 22, 2015
rvagg pushed a commit that referenced this pull request Sep 22, 2015
Logic for calling the passed in socket method and/or callback
was duplicated. This commit refactors the relevant code to
remove the redundancy.

PR-URL: #2769
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brendan Ashworth <[email protected]>
@rvagg rvagg mentioned this pull request Sep 22, 2015
@ahoym ahoym deleted the DRY-ClientRequest-_deferToConnect branch September 30, 2015 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants