net: multiple listen() events fail silently by eduardbme · Pull Request #13149 · nodejs/node · GitHub

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)

net

Description of change

Problem: It's possible to run listen() on a net.Server that's already listening to a port.
The result is silent failure, with the side effect of changing the _connectionKey and or _pipeName.

Solution: emit error if listen method called more than once.
close() method should be called between listen() method calls.

Refs: #8294
Fixes: #6190
Fixes: #11685

Previous PR #8419

hi there @jasnell @rvagg @addaleax @cjihrig @joyeecheung @evanlucas

@gibfahn have updated pr, please run CI
updated docs and moved away from accessors @bnoordhuis

CI: https://ci.nodejs.org/job/node-test-commit/10063/

There's no reason for the line break after *Note*: ...

*Note*: The `server.listen()` method...

during the first `listen()` call ...

Please avoid the use of you in docs :-)

Rather than introducing a new state variable for tracking, just look for the existence of the _handle. If it is not undefined or null, the server is listening, otherwise it is not.

What if the server._handle has been manually set to a non-listening wrap though?

Good point. But I think that the _ sign clearly says "okay, you know what you doing by accessing , sort of, private field. You are on your own way now".

Good idea but the implementation and docs need a bit of work. Added a few comments. Thank you for working on this.

@jasnell updated pr

The other thing I would recommend here is using the new internal/errors when introducing a new error.

@jasnell moved away from EINVAL error, added new one inside errors called ERR_SERVER_ALREADY_LISTEN and updated tests.

This is an interesting edge case. Would calling listen() twice be considered a programmer error? If so, we should throw, not emit an error. If we do stick with an emit(), should the PR return this for consistency with other returns in listen()?

@cjihrig I'd say it's a programmer error.

@cjihrig I think the theory is, if listen has already been called, whoever calls it is likely to have a error handler setup already and they should be notified. Although I think the second listen call should throw as well so the second caller can be notified ASAP

Excellent work, very clean PR! 👍

@eduardbcom this needs a rebase and as far as I see it the emit should be changed to throwing right away.

One comment, otherwise LGTM

@BridgeAR done

Nit: typo (can be fixed while landing though)

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

@gibfahn @cjihrig @joyeecheung @jasnell you might want to have another look as the emit now changed to a throw instead.

Still LGTM but I'd like to get more @nodejs/ctc reviews

LGTM

Drop the word "method" after server.close() in this sentence.

would be -> will be

Same comments apply to this paragraph and the one below.

Style nit, but could you move this to the previous line. It saves a level of indentation.

Problem: It's possible to run listen() on a net.Server that's already listening to a port. The result is silent failure, with the side effect of changing the connectionKey and or pipeName. Solution: throw an error if listen method called more than once. close() method should be called between listen() method calls. Refs: nodejs#8294 Fixes: nodejs#6190 Fixes: nodejs#11685

Thank you folks for your comments. Updated

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

LGTM if CI is happy

CI failures are unrelated, this can land.

Landed as b24e269.

Thanks @eduardbcom for your contribution!

Problem: It's possible to run listen() on a net.Server that's already listening to a port. The result is silent failure, with the side effect of changing the connectionKey and or pipeName. Solution: throw an error if listen method called more than once. close() method should be called between listen() method calls. Refs: #8294 Fixes: #6190 Fixes: #11685 PR-URL: #13149 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Problem: It's possible to run listen() on a net.Server that's already listening to a port. The result is silent failure, with the side effect of changing the connectionKey and or pipeName. Solution: throw an error if listen method called more than once. close() method should be called between listen() method calls. Refs: nodejs/node#8294 Fixes: nodejs/node#6190 Fixes: nodejs/node#11685 PR-URL: nodejs/node#13149 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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

jasnell reviewed May 22, 2017

View reviewed changes

jasnell requested changes May 22, 2017

View reviewed changes

eduardbme force-pushed the listenhasbeencalled branch 4 times, most recently from b07d6f5 to fbe11dc Compare May 28, 2017 12:15

jasnell reviewed May 29, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 29, 2017

eduardbme force-pushed the listenhasbeencalled branch 2 times, most recently from 068bcbf to c566e77 Compare June 22, 2017 11:07

jasnell approved these changes Jun 22, 2017

View reviewed changes

Trott mentioned this pull request Jul 10, 2017

net: multiple listen() events fail silently #6190

Closed

justsml approved these changes Aug 17, 2017

View reviewed changes

BridgeAR approved these changes Aug 27, 2017

View reviewed changes

eduardbme force-pushed the listenhasbeencalled branch from c566e77 to eadf97a Compare August 29, 2017 20:27

BridgeAR approved these changes Aug 30, 2017

View reviewed changes

mcollina approved these changes Aug 31, 2017

View reviewed changes

cjihrig approved these changes Aug 31, 2017

View reviewed changes

eduardbme force-pushed the listenhasbeencalled branch from eadf97a to 2cd52c9 Compare September 2, 2017 16:53

joyeecheung approved these changes Sep 3, 2017

View reviewed changes

mcollina closed this Sep 4, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

leonardoadame mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#332

Open

aliscco mentioned this pull request Dec 1, 2023

[Snyk] Security upgrade jest from 26.4.2 to 29.0.0 aliscco/alisco-node#924

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#1050

Open

leonardoadame mentioned this pull request Dec 3, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#477

Open

This was referenced May 14, 2024

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#879

Open

[Snyk] Security upgrade jest from 28.1.3 to 29.0.0 leonardoadame/node#918

Open

aliscco mentioned this pull request May 15, 2024

[Snyk] Security upgrade jest from 26.4.2 to 29.0.0 aliscco/alisco-node#1535

Open

This was referenced May 16, 2024

[Snyk] Security upgrade jest from 27.5.1 to 29.0.0 leonardoadame/node#946

Open

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#966

Open

aliscco mentioned this pull request May 17, 2024

[Snyk] Security upgrade jest from 26.4.2 to 29.0.0 aliscco/alisco-node#1603

Open

snyk-io Bot mentioned this pull request Oct 23, 2024

[Snyk] Fix for 5 vulnerabilities Graybar-codespace/node#93

Open

net: multiple listen() events fail silently by eduardbme · Pull Request #13149 · nodejs/node · GitHub

Uh oh!

Uh oh!

net: multiple listen() events fail silently - #13149

net: multiple listen() events fail silently#13149eduardbme wants to merge 1 commit intonodejs:masternodejs/node:masterfrom eduardbme:listenhasbeencallededuardbme/node:listenhasbeencalledCopy head branch name to clipboard

Conversation

eduardbme commented May 22, 2017

Uh oh!

eduardbme commented May 22, 2017

Uh oh!

gibfahn commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

eduardbme commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardbme commented Jun 22, 2017

Uh oh!

cjihrig commented Jun 22, 2017

Uh oh!

BridgeAR commented Jun 22, 2017

Uh oh!

joyeecheung commented Jun 24, 2017

Uh oh!

justsml left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 27, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

eduardbme commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 30, 2017

Uh oh!

BridgeAR commented Aug 30, 2017

Uh oh!

jasnell commented Aug 30, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardbme commented Sep 2, 2017

Uh oh!

joyeecheung commented Sep 3, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 4, 2017

Uh oh!

mcollina commented Sep 4, 2017

Uh oh!

mcollina commented Sep 4, 2017

net: multiple listen() events fail silently#13149
eduardbme wants to merge 1 commit into
nodejs:masterfrom
eduardbme:listenhasbeencalled