net: micro cleanups #12342

bnoordhuis · 2017-04-11T18:26:00Z

I had to wait for a couple of builds to finish so I figured I might as well go and do something useful.

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

mscdex · 2017-04-11T19:13:26Z

lib/net.js

@@ -497,7 +498,7 @@ Socket.prototype.destroySoon = function() {
 Socket.prototype._destroy = function(exception, cb) {
  debug('destroy');

-  function fireErrorCallbacks(self) {
+  function fireErrorCallbacks(self, exception, cb) {


I'm guessing this is done for performance, but does shadowing negate that?

You mean shadowing of variable/argument names? No, should not be a factor here.

jasnell · 2017-04-18T17:59:48Z

One failure in freebsd that might be related... trying again https://ci.nodejs.org/job/node-test-pull-request/7490/

bnoordhuis · 2017-04-18T19:45:59Z

java.lang.NoClassDefFoundError on the fedora22 bot, otherwise green. I'll go ahead and land this.

Pass arguments to fireErrorCallbacks() explicitly. Saves allocation an unnecessary closure context. PR-URL: nodejs#12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Don't call `Function#bind()` when a direct method call works just as well and is much cheaper. PR-URL: nodejs#12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Call internalConnect() directly when the target is an IP address. No delay is necessary because it defers any callbacks it makes. PR-URL: nodejs#12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Not necessary, not a good idea. PR-URL: nodejs#12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Split up Socket#connect() so that we don't call normalizeArgs() twice when invoking net.connect() or net.createConnection(). PR-URL: nodejs#12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Avoid unnecessary calls to require('dns') by caching the result of the first one. PR-URL: nodejs#12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Pass arguments to fireErrorCallbacks() explicitly. Saves allocation an unnecessary closure context. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Call internalConnect() directly when the target is an IP address. No delay is necessary because it defers any callbacks it makes. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Not necessary, not a good idea. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Split up Socket#connect() so that we don't call normalizeArgs() twice when invoking net.connect() or net.createConnection(). PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Avoid unnecessary calls to require('dns') by caching the result of the first one. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Pass arguments to fireErrorCallbacks() explicitly. Saves allocation an unnecessary closure context. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Call internalConnect() directly when the target is an IP address. No delay is necessary because it defers any callbacks it makes. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Not necessary, not a good idea. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Split up Socket#connect() so that we don't call normalizeArgs() twice when invoking net.connect() or net.createConnection(). PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Avoid unnecessary calls to require('dns') by caching the result of the first one. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Pass arguments to fireErrorCallbacks() explicitly. Saves allocation an unnecessary closure context. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Call internalConnect() directly when the target is an IP address. No delay is necessary because it defers any callbacks it makes. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Not necessary, not a good idea. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Split up Socket#connect() so that we don't call normalizeArgs() twice when invoking net.connect() or net.createConnection(). PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Avoid unnecessary calls to require('dns') by caching the result of the first one. PR-URL: #12342 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

addaleax · 2017-05-05T09:49:05Z

Revert for bb06add is open in #12852 so I’ll add dont-land labels

evanlucas · 2017-05-05T11:19:59Z

Also opened a revert for 571882c in #12854

It's important for people who monkey-patch `Socket.prototype.connect` that it's called by `net.connect` since it's not possible to monkey-patch `net.connect` directly (as the `connect` function is called directly by other parts of `lib/net.js` instead of calling the `exports.connect` function). Among the actors who monkey-patch `Socket.prototype.connect` are most APM vendors, the async-listener module and the continuation-local-storage module. Related: - nodejs#12342 - nodejs#12852

It's important for people who monkey-patch `Socket.prototype.connect` that it's called by `net.connect` since it's not possible to monkey-patch `net.connect` directly (as the `connect` function is called directly by other parts of `lib/net.js` instead of calling the `exports.connect` function). Among the actors who monkey-patch `Socket.prototype.connect` are most APM vendors, the async-listener module and the continuation-local-storage module. Related: - #12342 - #12852 PR-URL: #12861 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luca Maraschi <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jan Krems <[email protected]>

It's important for people who monkey-patch `Socket.prototype.connect` that it's called by `net.connect` since it's not possible to monkey-patch `net.connect` directly (as the `connect` function is called directly by other parts of `lib/net.js` instead of calling the `exports.connect` function). Among the actors who monkey-patch `Socket.prototype.connect` are most APM vendors, the async-listener module and the continuation-local-storage module. Related: - nodejs#12342 - nodejs#12852 PR-URL: nodejs#12861 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luca Maraschi <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jan Krems <[email protected]>

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Apr 11, 2017

cjihrig approved these changes Apr 11, 2017

View reviewed changes

evanlucas approved these changes Apr 11, 2017

View reviewed changes

lpinca approved these changes Apr 11, 2017

View reviewed changes

mscdex reviewed Apr 11, 2017

View reviewed changes

jasnell approved these changes Apr 12, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:12

benjamingr approved these changes Apr 15, 2017

View reviewed changes

bnoordhuis added 6 commits April 18, 2017 22:02

bnoordhuis force-pushed the net-micro-cleanups branch from a80ff83 to 5d06e5c Compare April 18, 2017 20:05

bnoordhuis closed this Apr 18, 2017

bnoordhuis deleted the net-micro-cleanups branch April 18, 2017 20:05

bnoordhuis merged commit 5d06e5c into nodejs:master Apr 18, 2017

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

addaleax added dont-land-on-v4.x labels May 5, 2017

watson mentioned this pull request May 5, 2017

net: ensure net.connect calls Socket connect #12861

Closed

3 tasks

github-actions bot mentioned this pull request Apr 24, 2024

Adding VersionStream for nodejs-22 wolfi-dev/os#17646

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: micro cleanups #12342

net: micro cleanups #12342

bnoordhuis commented Apr 11, 2017 •

edited

Loading

mscdex Apr 11, 2017

bnoordhuis Apr 11, 2017

jasnell commented Apr 18, 2017

bnoordhuis commented Apr 18, 2017

addaleax commented May 5, 2017

evanlucas commented May 5, 2017

net: micro cleanups #12342

net: micro cleanups #12342

Conversation

bnoordhuis commented Apr 11, 2017 • edited Loading

mscdex Apr 11, 2017

Choose a reason for hiding this comment

bnoordhuis Apr 11, 2017

Choose a reason for hiding this comment

jasnell commented Apr 18, 2017

bnoordhuis commented Apr 18, 2017

addaleax commented May 5, 2017

evanlucas commented May 5, 2017

bnoordhuis commented Apr 11, 2017 •

edited

Loading