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: DRY ClientRequest.prototype._deferToConnect #2769

Closed
wants to merge 3 commits into from
Closed

http: DRY ClientRequest.prototype._deferToConnect #2769

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
073ec80
http: DRY ClientRequest.prototype._deferToConnect
ahoym Sep 9, 2015
66ddd88
Prefer fn declaration and braceless if statements
ahoym Sep 9, 2015
1f20e30
Remove unnecessary semi-colon
ahoym Sep 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 😄

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()

if (method) {
self.socket[method].apply(self.socket, arguments_);
}
if (cb) { cb(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

if (typeof cb === 'function')
  cb();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are the absence of curly braces preferred?

I'm just wondering because I've seen plenty of cases of both in this file, and I'd like to conform to consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, core prefers to leave out braces in if statements. If there are any else conditions that require braces, then all branches use braces for consistency (not applicable here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, good to know.

Then on a similar vein, we should also do this above?

if (method)
  self.socket[method].apply(self.socket, arguments_);

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Yes :-)

};
var onSocket = function() {
if (self.socket.writable) {
if (method) {
self.socket[method].apply(self.socket, arguments_);
}
if (cb) { cb(); }
callSocketMethod();
} else {
self.socket.once('connect', function() {
if (method) {
self.socket[method].apply(self.socket, arguments_);
}
if (cb) { cb(); }
});
self.socket.once('connect', callSocketMethod);
}
};
if (!self.socket) {
Expand Down