net: support passing null to listen()

[cjihrig]

This commit fixes a regression around the handling of null as the port passed to Server#listen(). With this commit, null, undefined, and 0 have the same behavior, which was the case in Node 4.

Fixes: #14205

R= @sam-github@evanlucas

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

net

[cjihrig]

It may not be obvious, but because of the loop, this tests both cases that were removed below.

[refack]

What's the semverity?

[refack]

Tiny nit: could you swap the && clauses (for readability)

[refack]

Regarding semverity — IMHO should be patch
Ref: #14205 (comment)

[sam-github]

I tested this against test/parallel/test-net-listen-port-option.js from #14234 (6.x), and it has a different behaviour. 4.x and 6.x allow listen(null), but do not allow listen({port: null}), but with this change, that latter behaviour becomes allowed.

I am out of time for the day, but I also suspect that the true/false behaviour may be different. 8.x doesn't support true/false as aliases for 1/0 in listen(true or false) (this is good), but I think it might in listen({port: true or false}). I think accepting true or false as meaning 1 or 0 was a bug that should be (and maybe partially is?) in 8.x or as soon as is possible given the semver-majorness of the change. This doesn't look related to this PR.

[sam-github]

I confirmed, this introduces null being allowed not only as an argument to listen(), but also as an option to listen, making it incompatible with 6.x and 4.x:

core/node (pr/14221 $% u=) % ./out/Release/node -pe 'var s = require("http").createServer(function(){}).listen(undefined, function(){}).unref(); s.address()'{address: '::', family: 'IPv6', port: 32983 } core/node (pr/14221 $% u=) % nvm i 6 v6.11.1 is already installed. Now using node v6.11.1 (npm v3.10.10) core/node (pr/14221 $% u=) % node -v && node -pe 'var s = require("http").createServer(function(){}).listen(undefined, function(){}).unref(); s.address()' v6.11.1 internal/net.js:17 throw new RangeError('"port" argument must be >= 0 and < 65536'); 

[BridgeAR]

Ping @cjihrig

[cjihrig]

I'm OK with introducing listen(null) and listen({port: null}) both working because it's backward compatible. The option parsing for listen() is already overly complicated and convoluted. I don't really want to monkey with it any more than is necessary. So, if this change is going to be rejected, I'd rather close the PR.

[cjihrig]

Any takers on what to do with this one? I'd like to either land it or close it.

[BridgeAR]

I am somewhat against this because I think it is better to have less implicit behavior. It is just less error prone. I do not have such a strong opinion about it to block it though.

[BridgeAR]

The comment should reflect the new behavior.

[sam-github]

will leave review as a comment

[sam-github]

Sorry for the churn, re #14221 (comment)

I agree its consistent to have the argument value to mean the same thing whether it is provided as a positional argument, or the value of an option property.

LGTM

[cjihrig]

@gibfahn is there a problem with the lint job on CI? Linting is failing for me, despite passing locally. See https://ci.nodejs.org/job/node-test-linter/11939/console.

It looks like it's happening on other PRs too. See https://ci.nodejs.org/job/node-test-linter/11936/console (although I haven't confirmed if that passes linting locally).

EDIT: I confirmed with @mcollina that the second case does have linting errors. I'm still not sure about the linting for this PR though.

[gibfahn]

@cjihrig probably #15441 getting landed without CI. Let me check.

Run on master to confirm: https://ci.nodejs.org/job/node-test-linter/11941/console

EDIT: Actually that was green.

Also weird that it's now taking me 70s to rerun make lint on my machine, I'm sure it was substantially less before.

[cjihrig]

Here is a CI run: https://ci.nodejs.org/job/node-test-pull-request/10196/

There were no related failures, but the linter failed. It seems like @gibfahn got that straightened out because here is a CI run with no code changes where the linter passes: https://ci.nodejs.org/job/node-test-pull-request/10204/. Thanks @gibfahn

Landing this.

[gibfahn]

There were no related failures, but the linter failed. It seems like @gibfahn got that straightened out because here is a CI run with no code changes where the linter passes

nodejs/build#893

[MylesBorins]

Support for passing undefined was added to v6.x in caeee38, but we still don't have support for null

@cjihrig would you be willing to backport this?