net: multiple listen() events fail silently #13149

eduardbme · 2017-05-22T10:56:51Z

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

eduardbme · 2017-05-22T11:03:01Z

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

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

gibfahn · 2017-05-22T11:44:18Z

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

jasnell · 2017-05-22T14:38:47Z

doc/api/http.md

-*Note*: The `server.listen()` method may be called multiple times. Each
-subsequent call will *re-open* the server using the provided options.
+*Note*:
+The `server.listen()` method can be called again if and only if there was an error


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

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

jasnell · 2017-05-22T14:39:25Z

doc/api/http.md

-subsequent call will *re-open* the server using the provided options.
+*Note*:
+The `server.listen()` method can be called again if and only if there was an error
+during the first *listen* call or you explicitly called `server.close()`.


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

Please avoid the use of you in docs :-)

jasnell · 2017-05-22T14:42:37Z

lib/net.js

+    return this.emit('error', er);
+  }
+
+  this._serverListenHasBeenCalled = true;


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

jasnell

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

eduardbme · 2017-05-28T12:33:28Z

@jasnell updated pr

jasnell · 2017-05-29T21:41:36Z

lib/net.js

@@ -1404,6 +1404,12 @@ Server.prototype.listen = function() {
  var options = normalized[0];
  var cb = normalized[1];

+  if (this._handle) {
+    var er = new Error('listen method has been called more than once.');


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

eduardbme · 2017-06-22T11:13:53Z

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

cjihrig · 2017-06-22T16:25:48Z

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()?

BridgeAR · 2017-06-22T16:38:01Z

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

joyeecheung · 2017-06-24T08:55:20Z

@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

justsml

Excellent work, very clean PR! 👍

BridgeAR · 2017-08-27T00:14:43Z

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

BridgeAR

One comment, otherwise LGTM

eduardbme · 2017-08-29T20:28:43Z

@BridgeAR done

BridgeAR · 2017-08-30T12:54:07Z

test/parallel/test-net-server-call-listen-multiple-times.js

+  }));
+}
+
+// Thrid test.


Nit: typo (can be fixed while landing though)

BridgeAR · 2017-08-30T18:49:29Z

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

BridgeAR · 2017-08-30T18:50:53Z

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

jasnell · 2017-08-30T19:17:25Z

test/parallel/test-net-server-call-listen-multiple-times.js

+    code: 'ERR_SERVER_ALREADY_LISTEN',
+    type: Error,
+  }));
+}


nit... this can be changed to:

common.expectsError(() => server.listen(), { code: 'ERR_SERVER_ALREADY_LISTEN', type: Error });

jasnell · 2017-08-30T19:17:55Z

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

mcollina

LGTM

cjihrig · 2017-08-31T13:23:49Z

doc/api/http.md

-*Note*: The `server.listen()` method may be called multiple times. Each
-subsequent call will *re-open* the server using the provided options.
+*Note*: The `server.listen()` method can be called again if and only if there was an error
+during the first `server.listen()` call or `server.close()` method has been called.


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

cjihrig · 2017-08-31T13:24:15Z

doc/api/http.md

-subsequent call will *re-open* the server using the provided options.
+*Note*: The `server.listen()` method can be called again if and only if there was an error
+during the first `server.listen()` call or `server.close()` method has been called.
+Otherwise, an `ERR_SERVER_ALREADY_LISTEN` error would be thrown.


would be -> will be

cjihrig · 2017-08-31T13:24:49Z

doc/api/http.md

-*Note*: The `server.listen()` method may be called multiple times. Each
-subsequent call will *re-open* the server using the provided options.
+*Note*: The `server.listen()` method can be called again if and only if there was an error
+during the first `server.listen()` call or `server.close()` method has been called.


Same comments apply to this paragraph and the one below.

cjihrig · 2017-08-31T13:28:01Z

test/parallel/test-net-server-call-listen-multiple-times.js

+  const server = net.Server();
+
+  server.listen(common.mustCall(
+    () => {


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

eduardbme · 2017-09-02T16:55:48Z

Thank you folks for your comments. Updated

joyeecheung · 2017-09-03T03:08:09Z

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

joyeecheung

LGTM if CI is happy

mcollina · 2017-09-04T10:36:52Z

CI failures are unrelated, this can land.

mcollina · 2017-09-04T10:44:52Z

Landed as b24e269.

mcollina · 2017-09-04T10:45:17Z

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

jasnell reviewed 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

net: multiple listen() events fail silently #13149

net: multiple listen() events fail silently #13149

Conversation

eduardbme commented May 22, 2017

eduardbme commented May 22, 2017

gibfahn commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

eduardbme commented May 28, 2017

Choose a reason for hiding this comment

eduardbme commented Jun 22, 2017

cjihrig commented Jun 22, 2017

BridgeAR commented Jun 22, 2017

joyeecheung commented Jun 24, 2017

justsml left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 27, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

eduardbme commented Aug 29, 2017

Choose a reason for hiding this comment

BridgeAR commented Aug 30, 2017

BridgeAR commented Aug 30, 2017

Choose a reason for hiding this comment

jasnell commented Aug 30, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardbme commented Sep 2, 2017

joyeecheung commented Sep 3, 2017

joyeecheung left a comment

Choose a reason for hiding this comment

mcollina commented Sep 4, 2017

mcollina commented Sep 4, 2017

mcollina commented Sep 4, 2017