net: multiple listen() events fail silently #8419

eduardbme · 2016-09-06T10:48:43Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (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.

addaleax · 2016-09-06T10:51:05Z

Related: #8294 … I read that as meaning that the current behaviour is the intended one?

eduardbme · 2016-09-06T10:56:19Z

The issue #6190 for this PR still open.
Maybe documentation should be changed ?

Also, it should be more appropriate err code instead of er.code = 'EADDRINUSE';. Or we need to create one.

addaleax · 2016-09-06T10:57:35Z

oh, yeah. /cc @trevnorris @jasnell

thefourtheye · 2016-09-10T13:36:40Z

lib/net.js

+    var er = new Error('listen method has been called more than once.');
+    // maybe this is should be another code error
+    er.code = 'EADDRINUSE';
+    self.emit('error', er);


After emitting this error, why does it continue?

eduardbme · 2016-09-11T07:19:45Z

@thefourtheye Thank you for comments.
Have changed PR according to your comments.

eduardbme · 2016-09-16T16:37:59Z

Hi there, @addaleax @thefourtheye @trevnorris @jasnell just a quick reminder.

jasnell · 2016-09-20T17:00:56Z

Thanks for the reminder ping. I'll take another look at this a bit later on today.

eduardbme · 2016-09-27T09:08:54Z

Hi there, @addaleax @thefourtheye @trevnorris @jasnell just a quick reminder. Second one :)

thefourtheye · 2016-09-27T09:46:38Z

@eduardbcom Can you please rebase, so that we can take another look at the latest code?

ronkorving · 2016-09-27T10:00:40Z

It would be (have been?) nice if this could('ve) landed in Node 7.

jasnell · 2016-09-27T14:53:22Z

As a semver-major it can't get into v7 without CTC review and ok. We can try. Adding it to the ctc agenda.

addaleax · 2016-10-04T07:37:45Z

ping @eduardbcom … could you rebase this so that it can be ready for merging from your side? :)

eduardbme · 2016-10-04T08:42:54Z

@addaleax sorry for the delay, I don't exactly understand what you expect to receive. rebase to master ? thx.

gibfahn · 2016-10-04T13:36:17Z

@eduardbcom Yep, rebase against master to remove the merge conflicts github is complaining about.

rvagg · 2016-10-05T08:54:12Z

lib/net.js

@@ -1327,6 +1329,15 @@ function listen(self, address, port, addressType, backlog, fd, exclusive) {
 Server.prototype.listen = function() {
  var self = this;

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


are we using let in newly added code or are the existing var's in here keeping us consistent?

addaleax · 2016-10-05T20:40:26Z

From what I’ve gathered in the CTC meeting it seems like calling .listen() twice is currently problematic under basically all circumstances, so this LGTM and I’m +1 to landing on v7.x.

rvagg · 2016-10-05T20:42:30Z

CTC meeting:

@trevnorris raised that _handle is overwritten if you listen() a second time
@addaleax raised a possible concern with doc: document what happens when listen is called multiple times #8294 which states in the docs that you can listen() on the same socket multiple times. It should probably say more clearly that you can only listen() on an already closed socket.
There's also concern about whether close() is called in every situation when a socket is closed, in this PR it requires that path to unset the flag, is there any way a socket can be closed without that code path?

No negatives from the CTC meeting but it might need to come back next week or be handled async here.

jasnell · 2016-10-06T18:07:12Z

I'm +1 on landing it in v7

gibfahn · 2017-03-11T11:32:22Z

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

@nodejs/ctc this needs some review.

cjihrig · 2017-03-13T14:22:35Z

lib/net.js

@@ -1379,6 +1397,14 @@ Server.prototype.listen = function() {
  var options = normalized[0];
  const cb = normalized[1];

+  if (this[serverListenHasBeenCalled]) {
+    var er = new Error('listen method has been called more than once.');
+    er.code = 'ELISTENCALLEDTWICE';


I don't think we should create new error codes.

What error code should I use ?

We don't necessarily need to attach a code. If we really want one, EISCONN, EADDRNOTAVAIL, or maybe EALREADY are options.

cjihrig · 2017-03-13T14:24:40Z

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

+let err1 = false;
+let err2 = false;
+
+function case1() {


You don't need to use functions. You can create separate block scopes.

cjihrig · 2017-03-13T14:25:11Z

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

+function case1() {
+
+  const dummyServer = net.Server();
+  const server1 = net.Server();


You don't need server1 and server2 as different names. They are in different scopes. Just use server for both.

cjihrig · 2017-03-13T14:25:47Z

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

+  const dummyServer = net.Server();
+  const server1 = net.Server();
+
+  // run some server


Can you capitalize and punctuate comments. This might fit on a single line too.

cjihrig · 2017-03-13T14:26:37Z

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

+
+  // run some server
+  // in order to simulate EADDRINUSE error
+  dummyServer.listen(0, 'localhost', () => {


Please add common.mustCall() around this callback.

cjihrig · 2017-03-13T14:29:16Z

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

+
+  server2.on('error', common.mustCall(function(e) {
+    assert.strictEqual(e.code, 'ELISTENCALLEDTWICE');
+    err2 = true;


err2 isn't needed. This listener should only be called once, and is guaranteed by common.mustCall().

cjihrig · 2017-03-13T14:30:45Z

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

+        server1.close();
+      });
+    } else if (e.code === 'ELISTENCALLEDTWICE') {
+      err1 = true;


Instead of having err1, couldn't this just be an assertion?

cjihrig · 2017-03-13T14:31:05Z

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

+case1();
+case2();
+
+process.on('exit', function() {


I think this whole block could go away.

evanlucas · 2017-03-13T15:01:53Z

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 may be called one more time iff there was an error during the first *listen* call or you explicitly called `server.close()`. Otherwise, `ELISTENCALLEDTWICE` error would be emitted.


Can you break this line at 80 columns please?

evanlucas · 2017-03-13T15:02:08Z

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 may be called one more time iff there was an error during the first *listen* call or you explicitly called `server.close()`. Otherwise, `ELISTENCALLEDTWICE` error would be emitted.


Can you break this line at 80 columns please?

evanlucas · 2017-03-13T15:02:20Z

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 may be called one more time iff there was an error during the first *listen* call or you explicitly called `server.close()`. Otherwise, `ELISTENCALLEDTWICE` error would be emitted.


Can you break this line at 80 columns please?

evanlucas · 2017-03-13T15:02:52Z

lib/net.js

@@ -1379,6 +1397,14 @@ Server.prototype.listen = function() {
  var options = normalized[0];
  const cb = normalized[1];

+  if (this[serverListenHasBeenCalled]) {
+    var er = new Error('listen method has been called more than once.');
+    er.code = 'ELISTENCALLEDTWICE';


eduardbme · 2017-03-15T06:50:34Z

@evanlucas @cjihrig @gibfahn @Trott

thanks for comments. updated pr

bnoordhuis · 2017-03-15T08:15:56Z

doc/api/http.md

+*Note*:
+The `server.listen()` method may be called one more time iff there was an error
+during the first *listen* call or you explicitly called `server.close()`.
+Otherwise, `EALREADY` error would be emitted.


"Otherwise, an EALREADY error is emitted." - likewise below.

Also, I'd probably replace "may be called one more time " with "can be called again."

bnoordhuis · 2017-03-15T08:16:17Z

doc/api/net.md

-  subsequent call will *re-open* the server using the provided options.
+* The `server.listen()` method may be called one more time iff there was an error
+during the first *listen* call or you explicitly called `server.close()`.
+Otherwise, `EALREADY` error would be emitted.


Can you indent here?

bnoordhuis · 2017-03-15T08:17:47Z

lib/net.js

+      return this[serverHandle];
+    },
+    enumerable: true
+  });


I don't know how I feel about turning an internal property into an accessor... Why did you decide on this approach? Is it because of #8419 (comment)?

The main reason I decided to do it in this way it's because we need to set serverListenHasBeenCalled back to false when we close connection or some error has happened. And I have noticed that for both cases we call the same code to handle _handle field. Concretely:

this._handle.close(); this._handle = null;

So in order to avoid repetitions and do something like this every time we close this._handle

this._handle.close(); this._handle = null; this.serverListenHasBeenCalled = false

I decided to make this stuff in one place.

A big plus of this, is that if someone in a future will add some logic to handle error, they will not forget to set this.serverListenHasBeenCalled = false.

@bnoordhuis what are you thoughts on this now? Still conflicted?

I'd stick with simple properties. Accessors have overhead. If that means duplicating an assignment in two places, that's something I can live with.

bnoordhuis · 2017-03-15T08:21:20Z

lib/net.js

@@ -1379,6 +1397,14 @@ Server.prototype.listen = function() {
  var options = normalized[0];
  const cb = normalized[1];

+  if (this[serverListenHasBeenCalled]) {
+    var er = new Error('listen method has been called more than once.');
+    er.code = 'EALREADY';


This is in line with what libuv returns for client sockets but using EALREADY instead of EINVAL (the standard POSIX error for trying to rebind) is one of my little regrets there.

Thoughts, anyone? Consistency or conformity?

If you regret it, then we should probably go with EINVAL.

cjihrig · 2017-03-16T20:40:38Z

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

+  }));
+
+  server.listen(0, 'localhost', () => server.close());
+  server.listen(1, 'localhost');


Why 1 here?

Right, no reason for this.

Trott · 2017-03-18T03:32:34Z

Since this has received three CTC reviews since application of ctc-review label, I'm going to go ahead and remove that label. If anyone thinks it should stay, by all means, re-apply it.

jasnell · 2017-03-24T22:28:18Z

Ping @eduardbcom... some new comments have been added

eduardbme · 2017-03-26T10:55:58Z

@jasnell thanks for the reminder ping.

@bnoordhuis please check the comment for your question about the _handle property.

@cjihrig @evanlucas

updated pr. Thank you folks for your comments.

cjihrig · 2017-03-27T14:45:51Z

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

+    assert.strictEqual(e.code, 'EINVAL');
+  }));
+
+  server.listen(common.mustCall(


This can all be one line.

cjihrig · 2017-03-27T14:47:28Z

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

+// after first time is fail (without calling close method).
+
+// The second one is about calling a listen method twice.
+// It should be EALREADY but the first server listen call is working.


Should EALREADY be EINVAL?

cjihrig · 2017-03-27T14:47:57Z

lib/net.js

@@ -1373,6 +1391,14 @@ Server.prototype.listen = function() {
  var options = normalized[0];
  const cb = normalized[1];

+  if (this[serverListenHasBeenCalled]) {


Can you check === true here.

cjihrig · 2017-03-27T14:49:12Z

lib/net.js

  this._handle = null;
+


Can you remove the unnecessary whitespace changes (here and in other places).

eduardbme · 2017-03-27T19:17:02Z

thanks @cjihrig, updated

eduardbme · 2017-04-01T13:29:57Z

@jasnell @bnoordhuis @Trott @cjihrig @evanlucas quick reminder

bnoordhuis · 2017-04-03T11:25:39Z

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 iff there was an error


I forgot what our position was on using 'iff' in the documentation: yay or nay? git grep suggests nay.

My preference is to avoid it. Please use if and only if instead.

bnoordhuis · 2017-04-03T11:27:50Z

lib/net.js

+      return this[serverHandle];
+    },
+    enumerable: true
+  });


I'd stick with simple properties. Accessors have overhead. If that means duplicating an assignment in two places, that's something I can live with.

addaleax added net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 6, 2016

thefourtheye reviewed Sep 10, 2016
View reviewed changes

eduardbme force-pushed the listenhasbeencalled branch from f5b20c9 to a066225 Compare September 11, 2016 07:15

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

jasnell added the ctc-agenda label Sep 27, 2016

Trott mentioned this pull request Oct 3, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-10-05 #8915

Closed

rvagg reviewed Oct 5, 2016

View reviewed changes

jasnell added this to the 7.0.0 milestone Oct 6, 2016

mscdex added the ctc-review label Mar 11, 2017

cjihrig requested changes Mar 13, 2017

View reviewed changes

evanlucas suggested changes Mar 13, 2017

View reviewed changes

Trott mentioned this pull request Mar 13, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-15 nodejs/CTC#79

Closed

eduardbme force-pushed the listenhasbeencalled branch 2 times, most recently from 316f7ee to cd5396f Compare March 15, 2017 06:33

bnoordhuis requested changes Mar 15, 2017

View reviewed changes

cjihrig reviewed Mar 16, 2017

View reviewed changes

Trott removed the ctc-review label Mar 18, 2017

eduardbme force-pushed the listenhasbeencalled branch 2 times, most recently from e2f6e5e to 1488e56 Compare March 26, 2017 10:32

cjihrig requested changes Mar 27, 2017

View reviewed changes

eduardbme force-pushed the listenhasbeencalled branch from 1488e56 to 1533c61 Compare March 27, 2017 19:15

bnoordhuis requested changes Apr 3, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

eduardbme force-pushed the listenhasbeencalled branch 3 times, most recently from f1bfdbd to df659f0 Compare May 22, 2017 09:36

eduardbme closed this May 22, 2017

eduardbme force-pushed the listenhasbeencalled branch from f1bfdbd to 281de19 Compare May 22, 2017 09:59

eduardbme mentioned this pull request May 22, 2017

net: multiple listen() events fail silently #13149

Closed

4 tasks

This was referenced Mar 9, 2024

[Snyk] Upgrade @typescript-eslint/eslint-plugin from 6.21.0 to 7.0.1 B020239/node#2

Open

[Snyk] Upgrade @typescript-eslint/parser from 6.21.0 to 7.0.1 B020239/node#3

Open

net: multiple listen() events fail silently #8419

net: multiple listen() events fail silently #8419

Conversation

eduardbme commented Sep 6, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

addaleax commented Sep 6, 2016

eduardbme commented Sep 6, 2016 • edited Loading

addaleax commented Sep 6, 2016

Choose a reason for hiding this comment

eduardbme commented Sep 11, 2016

eduardbme commented Sep 16, 2016

jasnell commented Sep 20, 2016

eduardbme commented Sep 27, 2016

thefourtheye commented Sep 27, 2016

ronkorving commented Sep 27, 2016

jasnell commented Sep 27, 2016

addaleax commented Oct 4, 2016

eduardbme commented Oct 4, 2016 • edited Loading

gibfahn commented Oct 4, 2016

Choose a reason for hiding this comment

addaleax commented Oct 5, 2016

rvagg commented Oct 5, 2016

jasnell commented Oct 6, 2016

gibfahn commented Mar 11, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardbme commented Mar 15, 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

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

Trott commented Mar 18, 2017

jasnell commented Mar 24, 2017

eduardbme commented Mar 26, 2017 • edited by fhinkel Loading

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 Mar 27, 2017

eduardbme commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardbme commented Sep 6, 2016 •

edited

Loading

eduardbme commented Sep 6, 2016 •

edited

Loading

eduardbme commented Oct 4, 2016 •

edited

Loading

eduardbme commented Mar 26, 2017 •

edited by fhinkel

Loading