net: support passing null to listen() by cjihrig · Pull Request #14221 · nodejs/node

cjihrig · 2017-07-13T15:14:50Z

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 · 2017-07-13T15:15:28Z

test/parallel/test-net-server-listen-options.js

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

refack

What's the semverity?

refack · 2017-07-13T15:29:45Z

lib/net.js

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

refack · 2017-07-13T15:33:06Z

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

sam-github · 2017-07-14T18:33:51Z

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 · 2017-08-30T21:21:36Z

Ping @cjihrig

cjihrig · 2017-09-10T19:53:28Z

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 · 2017-09-20T03:07:30Z

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 · 2017-09-20T13:37:32Z

lib/net.js

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 · 2017-09-22T14:25:02Z

@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 · 2017-09-22T15:05:35Z

@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.

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: nodejs#14205 PR-URL: nodejs#14221 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

cjihrig · 2017-09-22T16:46:30Z

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 · 2017-09-22T16:56:34Z

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

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: nodejs/node#14205 PR-URL: nodejs/node#14221 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 PR-URL: #14221 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-10-17T18:29:22Z

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?

Early versions of Node.js 8 had a regression around the handling of `null` as the port passed to `Server#listen()`. For details see: nodejs/node#14221

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

cjihrig commented Jul 13, 2017

View reviewed changes

refack approved these changes Jul 13, 2017

View reviewed changes

lib/net.js Outdated

Copy link

Contributor

refack Jul 13, 2017

Choose a reason for hiding this comment

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

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

mscdex added dont-land-on-v4.x labels Jul 13, 2017

mscdex removed dont-land-on-v6.x labels Jul 13, 2017

cjihrig force-pushed the 14205 branch from 81fd88c to e612891 Compare July 13, 2017 15:48

evanlucas approved these changes Jul 13, 2017

View reviewed changes

sam-github approved these changes Jul 13, 2017

View reviewed changes

lpinca approved these changes Jul 13, 2017

View reviewed changes

jasnell approved these changes Jul 14, 2017

View reviewed changes

sam-github suggested changes Jul 17, 2017

View reviewed changes

BridgeAR reviewed Sep 20, 2017

View reviewed changes

lib/net.js Outdated

Copy link

Member

BridgeAR Sep 20, 2017

Choose a reason for hiding this comment

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

The comment should reflect the new behavior.

sam-github approved these changes Sep 20, 2017

View reviewed changes

sam-github suggested changes Sep 20, 2017

View reviewed changes

sam-github approved these changes Sep 20, 2017

View reviewed changes

cjihrig force-pushed the 14205 branch from e612891 to 3f06689 Compare September 22, 2017 00:59

cjihrig mentioned this pull request Sep 22, 2017

cluster: Can't passing http server to worker #15556

Closed

cjihrig force-pushed the 14205 branch from 3f06689 to 6e1e175 Compare September 22, 2017 13:49

cjihrig force-pushed the 14205 branch from 6e1e175 to 2b08367 Compare September 22, 2017 14:13

cjihrig force-pushed the 14205 branch from 2b08367 to 20259f9 Compare September 22, 2017 16:40

cjihrig merged commit 20259f9 into nodejs:master Sep 22, 2017

cjihrig deleted the 14205 branch September 22, 2017 16:47

cjihrig mentioned this pull request Sep 22, 2017

test: verify sending net.Server via cluster #15559

Closed

3 tasks

MylesBorins added dont-land-on-v6.x and removed dont-land-on-v6.x labels Oct 17, 2017

Uh oh!

Conversation

cjihrig commented Jul 13, 2017

Checklist

Affected core subsystem(s)

Uh oh!

cjihrig Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 13, 2017

Uh oh!

sam-github commented Jul 14, 2017

Uh oh!

sam-github left a comment • 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!

BridgeAR commented Aug 30, 2017

Uh oh!

cjihrig commented Sep 10, 2017

Uh oh!

cjihrig commented Sep 20, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Sep 22, 2017

Uh oh!

gibfahn commented Sep 22, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

sam-github left a comment •

edited

Loading

cjihrig commented Sep 22, 2017 •

edited

Loading

gibfahn commented Sep 22, 2017 •

edited

Loading