Skip to content

Commit

Permalink
net: don't create unnecessary closure
Browse files Browse the repository at this point in the history 
Don't call `Function#bind()` when a direct method call works
just as well and is much cheaper.

PR-URL: #12342
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
  • Loading branch information
@bnoordhuis
bnoordhuis committed Apr 18, 2017
1 parent 7b48303 commit 117b83c
Showing 1 changed file with 4 additions and 10 deletions.
14 changes: 4 additions & 10 deletions lib/net.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -858,26 +858,20 @@ function internalConnect(
var err;

if (localAddress || localPort) {
var bind;
debug('binding to localAddress: %s and localPort: %d (addressType: %d)',
localAddress, localPort, addressType);

This comment has been minimized.

Sign in to view

Copy link
@mscdex

mscdex Apr 18, 2017 •
edited
Loading

Contributor

@bnoordhuis I don't think it's safe to move the debug() here since localAddress is possibly assigned a different value below...

This comment has been minimized.

Sign in to view

Copy link
@mscdex

mscdex Apr 18, 2017

Contributor

Also, the debug message may be a lie if the addressType is invalid.

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Apr 18, 2017

Author Member

I suppose you're right but worst case it prints a false-y value instead of the default address and that doesn't seem so terrible.

That it logs when addressType is invalid I would consider a feature. =)

This comment has been minimized.

Sign in to view

Copy link
@mscdex

mscdex Apr 18, 2017 •
edited
Loading

Contributor

But if you're grepping debug output for the actual bound address it won't turn up. You'd have to know what false-y values to grep for in addition to 0.0.0.0.

This comment has been minimized.

Sign in to view

Copy link
@mscdex

mscdex Apr 18, 2017

Contributor

As far as it logging when addressType is invalid, I might agree with you if another debug() was added to the else block to indicate the bind didn't actually work. Again, grep-ability is useful, which is why I'd rather it only print if it actually was able to bind.

This comment has been minimized.

Sign in to view

Copy link
@bnoordhuis

bnoordhuis Apr 18, 2017

Author Member

It would have been nice if you'd mentioned this before I landed it. If you want to file a pull request moving it back, I'll review it.

This comment has been minimized.

Sign in to view

Copy link
@mscdex

mscdex Apr 18, 2017 •
edited
Loading

Contributor

I did not recall seeing this part of the PR. I will submit a PR.

if (addressType === 4) {
localAddress = localAddress || '0.0.0.0';
bind = self._handle.bind;
err = self._handle.bind(localAddress, localPort);
} else if (addressType === 6) {
localAddress = localAddress || '::';
bind = self._handle.bind6;
err = self._handle.bind6(localAddress, localPort);
} else {
self._destroy(new TypeError('Invalid addressType: ' + addressType));
return;
}

debug('binding to localAddress: %s and localPort: %d',
localAddress,
localPort);

bind = bind.bind(self._handle);
err = bind(localAddress, localPort);

if (err) {
const ex = exceptionWithHostPort(err, 'bind', localAddress, localPort);
self._destroy(ex);
Expand Down

0 comments on commit 117b83c

Please sign in to comment.