net: support passing undefined to listen()

[sam-github]

For consistency with 4.x and 8.x.

This commit also contains a forward port of
#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

Ref: #14205

Checklist

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

Affected core subsystem(s)

[sam-github]

@cjihrig What was happening is that assertPort() explicitly allows undefined as a valid value. However, toNumber() did not, and was converting undefined to false, and that then got passed to assertPort(), and threw. I think I fixed this in the right place, but perhaps you have other suggestions.

[sam-github]

@nodejs/lts PTAL, this is the LTS part of the fix for #14205

[lpinca]

This first argument should be port not an object.

[jasnell]

fwiw, this can be simplified to ....

net.Server().listen(true).on('error', common.expectsError({ code: 'EACCES' }));

[sam-github]

expectsError doesn't exist on 6.x, AFAICT

[jasnell]

ah! heh... good point ;-)

[sam-github]

@nodejs/lts PTAL

[sam-github]

ci: https://ci.nodejs.org/job/node-test-pull-request/9194/

[gibfahn]

@sam-github is this semver-minor? I guess if it restores previous behaviour that was then broken it's a bugfix.

[MylesBorins]

Waiting on confirmation if this is a bugfix or not. It will likely not make the v6.11.2-rc today, but willing to roll it into the next rc later this week if it is indeed a patch

[sam-github]

@MylesBorins This is a bug fix, see #14205 (comment), undefined could be passed in 4.x and 8.x, but can't in 6.x.

I want to land, but do I need one approval, or two? @cjihrig is the domain expert here and approved it, maybe his LGTM is all I need?

[jasnell]

I agree with bug fix.

[sam-github]

Landed in 0b17a45

[MylesBorins]

@sam-github This seems to be breaking windows CI so I'm backing it out of v6.11.2 for now

https://ci.nodejs.org/job/node-test-binary-windows/10189/RUN_SUBSET=0,VS_VERSION=vcbt2015,label=win10/

edit: for clarity I'm keeping it on staging for now, please let me know if you can figure out why we are getting time outs

[MylesBorins]

I'm backing this out of staging as the broken test is blocking CI on other PRS.

@sam-github please reopen, rebase, and try to get this passing on windows.

edit: here is a gist of the patch as you may have deleted the branch and links above could be removed in cache flush

https://gist.github.com/MylesBorins/f5ab7d9a23126c13c36ead9b2fa64f92

[MylesBorins]

ping @sam-github just a reminder this got backed out. I can't reopen this as you deleted your branch

[sam-github]

@MylesBorins restored branch, won't have time to look more at it until I'm back from vacation

[MylesBorins]

@nodejs/lts thoughts?

[jasnell]

Thoughts on what exactly? The test is still failing on Windows, correct? I'm good with this landing once that it resolved.

[sam-github]

I'm baffled that this has windows specific behaviour, and I'm looking into it.

starting with re-running ci: https://ci.nodejs.org/job/node-test-pull-request/10136/

[sam-github]

I figured this out, it's because net.Server().listen(1) works on Windows. I'll fix the test.

[sam-github]

ci: https://ci.nodejs.org/job/node-test-pull-request/10145/

[sam-github]

@MylesBorins last ci passed except for lint (fixed) and arm (probably broken), this should pass:

ci: https://ci.nodejs.org/job/node-test-pull-request/10148/

[MylesBorins]

landed in caeee38

[MylesBorins]

@cjihrig should we hold off on landing this until the bugs are all fixed in 8.x?

[cjihrig]

Just reiterating from IRC. I believe this should stay in the release if everything is passing. The bugs on 8.x in question are slightly different (#14205 (comment)).