net: rename internal functions for readability #11796

joyeecheung · 2017-03-11T04:29:47Z

The current namings are a bit confusing and hard to be searched for.

Rename listen to listenInCluster
Rename _listen2 to _setUpListenHandle
Remove _listen since it's a one-liner only used in one place
Correct comments in server.listen

Also Socket.prototype.listen doesn't seem to be tested nor documented, added a FIXME.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

net

joyeecheung · 2017-03-11T04:37:42Z

lib/net.js

Not sure if _listen2 should be simply removed (would anyone rely on a method with a name like this?)..._setUpListenHandle can be put into a internal symbol, though.

https://github.com/search?l=JavaScript&p=1&q=%22._listen2%22&ref=searchresults&type=Code&utf8=%E2%9C%93 shows a few uses, but nothing major I suppose.

Removing it would likely (unfortunately) require a semver-major and a deprecation cycle.

@jasnell I'll keep it and make a following doc-deprecate PR.

jasnell · 2017-03-14T23:46:30Z

lib/net.js

It may just be me but setUpListenHandle looks odd. Perhaps setupListenHandle would be better?

jasnell · 2017-03-14T23:47:28Z

lib/net.js

Can this comment be moved to the next line on it's own?

jasnell · 2017-03-14T23:48:26Z

lib/net.js

I know it was already like this, but can you split this into two lines?

const dns = require('dns'); dns.lookup(...)

jasnell

LGTM with a few suggestions

joyeecheung · 2017-03-15T02:53:32Z

Aside: the new relic agent and the opbeat agent are wrapping _listen2. I'll make a separate doc deprecate PR and cc the diagnostics folks..

joyeecheung · 2017-03-23T07:35:12Z

Addressed comments from @jasnell . I decided to leave _setUpListenHandle exposed because this might be useful for APM agents. cc @nodejs/diagnostics

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

jasnell · 2017-03-23T15:28:31Z

Given that _listen2 is still exposed as an alias for _setUpListenHandle, there is really no reason to also expose _setUpListenHandle -- in fact it's likely something we shouldn't do given that it's intended to be an internal function.

joyeecheung · 2017-03-24T05:16:33Z

@jasnell Yeah...thinking again if _listen2 has to be exposed for wrapping purposes, then the alias can be left as-is.

joyeecheung · 2017-03-24T06:29:24Z

I just realized if we stopped calling _listen2 in our code, the agents will be broken anyway, because they wrap _listen2 under the expectation that this method will be called by Node and they will have a chance to execute their own code in the wrapper. Therefore I only renamed the function declaration and still call server._listen2 to make the behavioral change minimal in this PR.

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

watson · 2017-03-24T09:05:02Z

@joyeecheung thanks for thinking of the APM vendors 😄

As far as I can see you ended up aliasing _setUpListenHandle as _listen2, but still call _listen2 internally - so existing code expecting a _listen2 function to be called should still work for now 👍

Both New Relic and Opbeat (and most likely other vendors as well) base our patching of Node core on the async-listener module. I'm not sure how many APM vendors merge in new changes from this module though (as we usually have to modify the module quite a bit to optimize the performance to our use-case). But in any case I've opened a PR in async-listener (othiym23/async-listener#103) to make sure it patches _setUpListenHandle instead of of _listen2 if available.

I fear that the new code is a little harder to understand than the old, since the code is now calling a legacy alias, which to outsiders might seem pretty weird.

So the reason why I want to get this into the hands of APM vendors ASAP is because I fear that it's just a matter of time before someone cleans up the code or by accident just calls _setUpListenHandle directly. This will break our instrumentation of course.

* Rename listen to listenInCluster * Rename _listen2 to _setupListenHandle * Remove _listen since it's a one-liner only used in one place * Correct comments in server.listen

joyeecheung · 2017-04-01T16:39:23Z

git am failed due to conflicts...rebased and last CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7151/

joyeecheung · 2017-04-01T17:39:55Z

Landed in 0ea4570, thanks!

* Rename listen to listenInCluster * Rename _listen2 to _setupListenHandle * Remove _listen since it's a one-liner only used in one place * Correct comments in server.listen PR-URL: #11796 Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung · 2017-04-01T17:40:36Z

EDIT: posted the wrong commit hash, it's 0ea4570

* Rename listen to listenInCluster * Rename _listen2 to _setupListenHandle * Remove _listen since it's a one-liner only used in one place * Correct comments in server.listen PR-URL: nodejs#11796 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-04-18T23:24:23Z

this should likely be backported

MylesBorins · 2017-05-15T15:42:28Z

ping

joyeecheung · 2017-10-17T16:50:33Z

Depends on #11667 which is don't-land-on-v6.x

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

joyeecheung commented Mar 11, 2017

View reviewed changes

jasnell reviewed Mar 14, 2017

View reviewed changes

lib/net.js Outdated

Copy link

Member

jasnell Mar 14, 2017

Choose a reason for hiding this comment

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

Can this comment be moved to the next line on it's own?

jasnell reviewed Mar 14, 2017

View reviewed changes

jasnell approved these changes Mar 14, 2017

View reviewed changes

joyeecheung force-pushed the rename-net-listen branch from 979c5e2 to e382b3d Compare March 23, 2017 07:33

joyeecheung force-pushed the rename-net-listen branch 3 times, most recently from af8de08 to 307480f Compare March 24, 2017 06:24

jasnell approved these changes Mar 24, 2017

View reviewed changes

watson mentioned this pull request Mar 24, 2017

Handle rename of net.Server.prototype._listen2 othiym23/async-listener#103

Merged

net: rename internal functions for readability

a24ab8f

* Rename listen to listenInCluster * Rename _listen2 to _setupListenHandle * Remove _listen since it's a one-liner only used in one place * Correct comments in server.listen

joyeecheung force-pushed the rename-net-listen branch from 307480f to a24ab8f Compare April 1, 2017 16:38

joyeecheung closed this Apr 1, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the lts-watch-v6.x label Apr 18, 2017

MylesBorins added the backport-requested-v6.x label May 15, 2017

joyeecheung self-assigned this May 16, 2017

joyeecheung added dont-land-on-v6.x and removed backport-requested-v6.x labels Oct 17, 2017

joyeecheung mentioned this pull request Oct 17, 2017

net: check EADDRINUSE after binding localPort #15097

Closed

3 tasks

joyeecheung removed their assignment Oct 17, 2017

Uh oh!

net: rename internal functions for readability #11796

net: rename internal functions for readability #11796

Uh oh!

Conversation

joyeecheung commented Mar 11, 2017

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

silverwind Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Mar 15, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Mar 23, 2017

Uh oh!

jasnell commented Mar 23, 2017

Uh oh!

joyeecheung commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Mar 24, 2017

Uh oh!

watson commented Mar 24, 2017

Uh oh!

joyeecheung commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Apr 1, 2017

Uh oh!

MylesBorins commented Apr 18, 2017

Uh oh!

MylesBorins commented May 15, 2017

Uh oh!

joyeecheung commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasnell Mar 14, 2017 •

edited

Loading

joyeecheung commented Mar 15, 2017 •

edited

Loading

joyeecheung commented Mar 24, 2017 •

edited

Loading

joyeecheung commented Apr 1, 2017 •

edited

Loading

joyeecheung commented Apr 1, 2017 •

edited

Loading