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: refactor Server.prototype.listen #4039

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
@jscissr
Contributor

jscissr commented Nov 26, 2015

This PR simplifies Server.prototype.listen, removing some redundancy and inconsistency.

Because listen and connect have a similar function signature, normalizeConnectArgs can be reused for listen.

listenAfterLookup renamed to lookupAndListen for consistency with lookupAndConnect, and moved out of Server.prototype.listen.

Functional differences

To improve consistency, I actually made some small functional changes:

  • port was checked with this complicated code if set in a config object:

    if (typeof h.port === 'number' || typeof h.port === 'string' ||
        (typeof h.port === 'undefined' && 'port' in h)) {
      // Undefined is interpreted as zero (random port) for consistency
      // with net.connect().
      if (typeof h.port !== 'undefined' && !isLegalPort(h.port))
        throw new RangeError('"port" option should be >= 0 and < 65536: ' +
                             h.port);
      …
    

    But if set as first argument, it was only coerced to a number with var port = toNumber(arguments[0]);.
    Now, it is consistently using the first way, toNumber is not used anymore.

    However this is a breaking change, because before invalid ports resulted in using a random port instead of throwing.

  • addressType was sometimes null and sometimes 4, if no address is given.
    Now it is always 4.

  • host was checked like this:

    if (arguments[1] !== undefined &&
        typeof arguments[1] !== 'function' &&
        typeof arguments[1] !== 'number')
    

    Now it is checked with if (typeof arguments[1] === 'string'), this is consistent with connect.

@Trott Trott added the net label Nov 26, 2015

@Trott

This comment has been minimized.

Member

Trott commented Nov 26, 2015

@jscissr

This comment has been minimized.

Contributor

jscissr commented Nov 26, 2015

The test sequential/test-net-server-address.js failed, because of the first functional difference mentioned above.
-1 is not a valid port; but before, it was simply changed to false in toNumber, which means a random port.
With this PR however, an error is thrown.

The solution is either to change the failing test, or reintroduce the inconsistency.
What do you think?

@cjihrig

View changes

lib/net.js Outdated
@@ -846,12 +846,16 @@ function connect(self, address, port, addressType, localAddress, localPort) {
}
// Check that the port number is not NaN when coerced to a number,
// is an integer and that it falls within the legal range of port numbers.
// Check that port is undefined, a number or a string.

This comment has been minimized.

@cjihrig

cjihrig Nov 26, 2015

Contributor

I understand why you made the changes, but it seems odd that this function accepts undefined as a valid port, and also that sometimes it returns a boolean and other times throws.

This comment has been minimized.

@jscissr

jscissr Nov 26, 2015

Contributor

It could be split into two functions: One that checks the type and one that does the coercion to number (port | 0) and throws if not valid.

@jscissr jscissr force-pushed the jscissr:simpler-listen branch 2 times, most recently Nov 27, 2015

@jscissr

This comment has been minimized.

Contributor

jscissr commented Nov 27, 2015

I made some updates, the CI should pass now.
I had to change a test, which means this PR is a breaking change (but only in edge cases).

@Trott

This comment has been minimized.

Member

Trott commented Nov 27, 2015

This change would remove a test introduced in 57c5655. /cc @indutny @raymondfeng

Even if the change should happen (about which I have no opinion, at least right now), the test should probably be modified so that it checks the new expected behavior rather than deleting it.

@jscissr

This comment has been minimized.

Contributor

jscissr commented Nov 27, 2015

But then I think it should be moved to parallel/test-net-listen-port-option.js.

@jscissr jscissr force-pushed the jscissr:simpler-listen branch 2 times, most recently Nov 27, 2015

@jscissr

This comment has been minimized.

Contributor

jscissr commented Dec 2, 2015

The problem is this: listen(port) treated illegal ports differently than listen({port: port}), connect(port) and connect({port: port}). This PR makes listen(port) behave like the others, but that is a breaking change.
More precisely, fractions (80.5) were truncated to integers, and invalid ports converted to 0 meaning random port. These cases now would throw errors.

I don't believe this is a big deal. I don't think anybody uses fractions in the port, and I guess that most people don't want to listen on a random port. And if they do, they will probably omit the port or use 0 as the docs say. But if they use something else, it should be easy to fix, as you get a clear error message and can simply follow the stack trace.
However, there was a test that used a port of -1.

What do you think?

@Trott

This comment has been minimized.

Member

Trott commented Dec 2, 2015

The isLegalPort() function that you wish to change appears to have been authored by @bnoordhuis in 480b482 in order to deal with an API inconsistency. Since that also is the motivation for your proposed change, perhaps he can offer an opinion as to the risk and value of the modifications here.

@jasnell jasnell added the semver-major label Dec 3, 2015

@jasnell

This comment has been minimized.

Member

jasnell commented Dec 3, 2015

Marking as semver-major because of the functional changes. It's possible we can downgrade this to semver-minor tho.

@jscissr jscissr closed this Dec 3, 2015

@jscissr jscissr reopened this Dec 3, 2015

@Trott Trott force-pushed the nodejs:master branch to 082cc8d Dec 27, 2015

@jscissr jscissr force-pushed the jscissr:simpler-listen branch Jan 15, 2016

@jscissr

This comment has been minimized.

Contributor

jscissr commented Jan 15, 2016

rebased

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 15, 2016

@jscissr ... thank you :-) I'll queue this up for a review shortly. Sorry that it's taken so long :-/

@jscissr

This comment has been minimized.

Contributor

jscissr commented Jan 15, 2016

Thanks @jasnell! In case you don't make a new major version anytime soon, I could also split the refactoring from the (breaking) consistency improvement, which means just a few extra lines that could be removed in the next major.

@estliberitas estliberitas force-pushed the nodejs:master branch 2 times, most recently to c7066fb Apr 26, 2016

jscissr added some commits Aug 12, 2016

Move listenAfterLookup out of listen
* move `listenAfterLookup` out of `Server.prototype.listen`
* rename `listenAfterLookup` to `lookupAndListen` for consistency with
`lookupAndConnect`
* self -> this
Use normalizeConnectArgs in listen
This is mostly preparation, `options` will be used later.
Only deal with options object
This removes the ifs directly dealing with arguments.
return typeof cb === 'function' ? [options, cb] : [options];
if (typeof cb !== 'function')
cb = null;
return [options, cb];

This comment has been minimized.

@mscdex

mscdex Aug 16, 2016

Contributor

The comment for normalizeArgs() needs to be updated now too.

lib/net.js Outdated
// Bind to a random port.
options.port = 0;
}
// The third optional argument is the backlog size.
// When the ip is omitted it can be the second argument.
var backlog = toNumber(arguments[1]) || toNumber(arguments[2]);
var backlog = toNumber(args[1]) || toNumber(args[2]);

This comment has been minimized.

@mscdex

mscdex Aug 16, 2016

Contributor

May want to do args.length checking before accessing [1] or [2].

This comment has been minimized.

@jscissr

jscissr Aug 16, 2016

Contributor

Do you mean like this?

var backlog = toNumber(args.length > 1 && args[1]) || toNumber(args.length > 2 && args[2]);

But is it really necessary / worth it to optimize this function? I don't think it's one of the hot functions in an usual node application, you call listen once during startup and not hundreds of times a second. What is your opinion?

This comment has been minimized.

@jscissr

jscissr Aug 29, 2016

Contributor

ping @mscdex

This comment has been minimized.

@mscdex

mscdex Sep 8, 2016

Contributor

I'd still prefer it for consistency.

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 8, 2016

Ping @mscdex, @jscissr and @mcollina ... if we're going to get this into v7 then it would need to land before the morning of Monday Sept 12th. I'm going to be cutting the v7.x branch that morning to start the beta process.

@mcollina

This comment has been minimized.

Member

mcollina commented Sep 8, 2016

I'm LGTM. @mscdex can you check if it is ok for you?

@mcollina

This comment has been minimized.

Member

mcollina commented Sep 12, 2016

@nodejs/collaborators Can we get some other quick LGTM?

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

@jasnell I'll like to get this landed for v7.

@mcollina

This comment has been minimized.

Member

mcollina commented Sep 12, 2016

CI is passing.

// It is the same as the argument of Socket.prototype.connect().
function normalizeConnectArgs(args) {
// This is also used by Server.prototype.listen().

This comment has been minimized.

@cjihrig

cjihrig Sep 12, 2016

Contributor

This should probably say that it is used by listen() and connect(). Just saying "also" doesn't mean much when the "connect" part is removed.

This comment has been minimized.

@mcollina

mcollina Sep 12, 2016

Member

@cjihrig I can fix the comment when I land. Is LGTM apart from this?

@thekemkid

This comment has been minimized.

Member

thekemkid commented Sep 12, 2016

This is getting a LGTM from me, if @mcollina will fix the comment when landing

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 12, 2016

@mcollina ... ok, I'll be cutting the branch right about at noon pacific today so there's still some time.

the change LGTM

@mcollina

This comment has been minimized.

Member

mcollina commented Sep 12, 2016

Landed as fd6af98

@mcollina mcollina closed this Sep 12, 2016

mcollina added a commit that referenced this pull request Sep 12, 2016

net: refactor Server.prototype.listen
This PR simplifies Server.prototype.listen, removing some redundancy and
inconsistency. Because listen and connect have a similar function signature,
normalizeConnectArgs can be reused for listen.
listenAfterLookup renamed to lookupAndListen for consistency with
lookupAndConnect, and moved out of Server.prototype.listen.

PR-URL: #4039
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Glen Keane <glenkeane.94@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@jscissr jscissr deleted the jscissr:simpler-listen branch Sep 12, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jan 15, 2017

It would appear this has broken passing null to listen

https://github.com/yoshuawuyts/crash-reporter-service/blob/master/tests/lib.js#L40

Was that intended behavior? If not we should land a fix

edit:

ref --> https://twitter.com/yoshuawuyts/status/820527120740913154

@mcollina

This comment has been minimized.

Member

mcollina commented Jan 15, 2017

The main discussion is in: #4039 (comment).

Yes it was a wanted fix, as I think that we should validate ports in a consistent manner. Specifically, in node v6 passing null as a port in the option object throws, but not when passing it as an argument.
See the change in the test: https://github.com/nodejs/node/pull/4039/files#diff-8900a04533ee1d4f9930c8ac100a2a96L24.

It has also passed a long amount of CITGM runs for all the v7 releases.

@sam-github

This comment has been minimized.

Member

sam-github commented Jan 16, 2017

+1 for consistency in validation, but I think passing null as port in option object should not throw.

I would expect calling .listen(function(){}), .listen(null, function(){}) , .listen(undefined, function(){}) , .listen({}, function(){}), .listen({port: null}, function(){}), .listen({port: undefined}, function(){})`, and all the previous when called without a callback, should have same behaviour, and the behaviour should be to treat it as "port argument was not provided (use default)".

Basically, in an options object, we should not distinguish between o{}, o={opt:null}, and o={opt:undefined}, since in all those three, o == null and o == undefined.

@mcollina

This comment has been minimized.

Member

mcollina commented Jan 17, 2017

@sam-github I'm not convinced about that. We support undefined, because that's what we get when something is not specified. In order to get null, it would have to be explicitly set. IMHO it's more robust against programmer errors in this way. It didn't break a lot of the community either, as what was mentioned was new software.

@ronkorving

This comment has been minimized.

Contributor

ronkorving commented Jan 17, 2017

@mcollina Imagine configuration coming out of node-config YAML files, and default.yaml has a port which is overridden by mcollina.yaml (NODE_ENV=mcollina) which sets port to null in order to turn off the default. Very normal use case imho.

@mcollina

This comment has been minimized.

Member

mcollina commented Jan 17, 2017

@ronkorving why do not set it to 0? For all valid use cases, this is typed by a human. And 0 is a much more sensible value than null from a SO perspective.

@sam-github

This comment has been minimized.

Member

sam-github commented Jan 17, 2017

@mcollina its easy to get keys set, config files, default handling, etc. Object.assign, util._extend can both clear keys by setting them to undefined, for example, but can't delete keys.

Can you explain why you think its more robust?

I've found the exact opposite. In Javascript, o={}, o={opt:null}, and o={opt:undefined}, o.opt == null and o.opt == undefined. Its is possible to distinguish between property not set, and property set with value of undefined, but you have to write extra code to do so, so APIs that distinguish between them are harder to document, implement, and use, and lead to subtle faillures in my experience.

@ronkorving

This comment has been minimized.

Contributor

ronkorving commented Jan 18, 2017

@mcollina Setting it to 0 is very explicitly setting it to that value. Trying to unset it so it uses whatever the default may be (in this case 0) has different semantics. You can state the first is always superior, but I guess my point is that there are various ways to look at this (of which at least one case can run into the null problem).

@mcollina

This comment has been minimized.

Member

mcollina commented Jan 18, 2017

@sam-github I generally think that massively overloaded method are extremely hard to maintain, optimize and keep secure. In this particular case, null was never documented as a possible value in the first place, and an accident of our implementation.

If it was for me, I would remove the default value for port, because it just leads to confusion.

Having said so, fire a PR and we will discuss it there. Even if I disagree, it seems a lot of people care about passing null as a port, so I'll be happy to review that change.

@sam-github

This comment has been minimized.

Member

sam-github commented Jan 18, 2017

Much of node's API is undocumented, unfortunately, so its hard to argue intentions.

I agree with you on massively overloaded methods, but not here.

if ("PORT" in process.env)
  server.listen(process.env.PORT, ...) // use the port
else
  server.listen(...) // use the default

is, IMO, unfortunate, I'd rather

server.listen(process.env.PORT, ...) // if the env var is not defined, you get the default behaviour

This pattern shows up quite often when loading config, not just env vars, which is why its important to base behaviour on a key's value, not its presence, its why:

var config = {}; // probably read from a config file or DB, note there is no PORT
server.listen({port: config.port}, .. // options.port exists, but value is undefined

should be the same as

server.listen({}, ..  // options.port key does not exist
@mcollina

This comment has been minimized.

Member

mcollina commented Jan 19, 2017

@sam-github

Just to clarify, this does not work in v6, but it does in v7 with this very change:

var net = require('net')

net.createServer().listen(process.env.PORT, function () {
  console.log('server listening on', this.address())
})

also

var config = {}
console.log(config.port) // => undefined
server.listen(config.port) // you get the default behavior

config.port = null
console.log(config.port) // => null
server.listen(config.port) // it throws, because null is not a valid port

IMHO it's working as you want, with the added benefit of preventing bad values you would have to come from somewhere unexpectedly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment