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

net: rename internal functions for readability #11796

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Mar 11, 2017
lib/net.js Outdated

Server.prototype._setUpListenHandle = setUpListenHandle;
Server.prototype._listen2 = setUpListenHandle; // legacy alias
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

lib/net.js Outdated

Server.prototype._listen2 = function(address, port, addressType, backlog, fd) {
debug('listen2', address, port, addressType, backlog, fd);
function setUpListenHandle(address, port, addressType, backlog, fd) {
Copy link
Member

@jasnell jasnell Mar 14, 2017

Choose a reason for hiding this comment

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

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

lib/net.js Outdated
exclusive = !!exclusive;

if (!cluster) cluster = require('cluster');

if (cluster.isMaster || exclusive) {
self._listen2(address, port, addressType, backlog, fd);
if (cluster.isMaster || exclusive) { // Will create a new handle
Copy link
Member

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?

lib/net.js Outdated
return this;
}

throw new Error('Invalid listen argument: ' + util.inspect(options));
};

function lookupAndListen(self, port, address, backlog, exclusive) {
require('dns').lookup(address, function doListening(err, ip, addressType) {
require('dns').lookup(address, function doListen(err, ip, addressType) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with a few suggestions

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 15, 2017

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
Copy link
Member Author

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
Copy link
Member

jasnell commented Mar 23, 2017

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
Copy link
Member Author

joyeecheung commented Mar 24, 2017

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

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

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
Copy link
Member

watson commented Mar 24, 2017

@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
Copy link
Member Author

joyeecheung commented Apr 1, 2017

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

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 1, 2017

Landed in 0ea4570, thanks!

@joyeecheung joyeecheung closed this Apr 1, 2017
joyeecheung added a commit that referenced this pull request Apr 1, 2017
* 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 <[email protected]>
@joyeecheung
Copy link
Member Author

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

@jasnell jasnell mentioned this pull request Apr 4, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
* 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 <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

this should likely be backported

@MylesBorins
Copy link
Contributor

ping

@joyeecheung
Copy link
Member Author

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

@joyeecheung joyeecheung removed their assignment Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants