net: multiple listen() events fail silently

[trevnorris]

• Version: >= v4.x
• Platform: All
• Subsystem: net

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.

More expected behavior would be to emit an error on the server's 'error' event handler.

Example test:

const assert = require('assert');
const net = require('net');
const server = net.createServer();
server.on('error', () => {
  process._rawDebug('server had an error');
});
server.listen(8080).listen(8081);
process.on('exit', () => {
  assert.equal(server.address().port, 8080);
  const key = server._connectionKey;
  assert.equal(key.substr(key.lastIndexOf(':')), ':8080');
});

[geppy]

I'd like to work this one. Reading through the CONTRIBUTING doc now.

[trevnorris]

@geppy Awesome. If you get a PR up, even if it's WIP, cc me and I'll be happy to help you out.

[bnoordhuis]

More expected behavior would be to emit an error on the server's 'error' event handler.

I think it makes more sense to throw an exception. Calling listen() repeatedly likely means there's a logic error in the application.

[trevnorris]

@bnoordhuis there's a potential timing issue. currently if you call .listen() twice, and each time pass a hostname, it will send out a dns request. which doesn't fill _handle until later. allowing the instance to think that listen() hasn't yet been called.

i think that'd be as simple as storing a _listenHasBeenCalled flag on the instance so we can throw immediately, but haven't looked at that in much detail.

[justsml]

Hi @trevnorris - I did a minimal implementation.

I have a few questions though:

1. Is it important for the server._connectionKey or server.address() to (always) be set before the error throws?
2. Should I move the throw/error closer to the DNS lookup?
3. Should property Server.listening only check this._handle - or should it also test this._listenHasBeenCalled

[justsml]

Hey @geppy - I just re-read this after the weekend & wanted to say I didn't mean to step on your toes!

This would be my first contribution into the core nodejs project. (I've only helped on the website side of things ... so far.)

I started on this after seeing @trevnorris 's tweet - but stalled after you 'called it'.
When I saw Trevors latest comment about _listenHasBeenCalled I jumped on it as I (essentially) already implemented such a pattern.

That said, please don't let me stop you from doing a PR - the more the merrier. 💯

Hope everyone had a good weekend!!!

[geppy]

@justsml Okay, thanks! I'll go ahead and file my PR when I get home.

[saghul]

I guess the case in which someone calls listen more than once to change the backlog is too weird to consider, right?

[bnoordhuis]

I'm undecided. I've been programming UNIX services for twenty years now and I've never had to adjust the backlog dynamically; it seems like a very obscure feature. Perhaps we can support it if it's almost free to do so implementation/maintenance-wise, but otherwise I wouldn't bother.

I am admittedly responsible for making it adjustable in libuv but my rationale at the time can be summarized as "why not?".

[saghul]

@bnoordhuis I defer to your good judgement and experience here, I just thought I'd bring it up just in case :-)

[trevnorris]

@bnoordhuis My main concern here is that calling .listen() twice overwrites the ._handle. Thus preventing the user a way to close the server, unless they use this from a server's event. Here's an example of strange behavior:

const server = require('net').createServer(function(c) {
  console.error(this.address());
}).listen(8080, 'localhost');

server.listen(8081);

Then run nc -v localhost 8080 you'll see the connection failed, and the connection handler is placed on port 8081. But if you change it to .listen(8080) you'll see that you can successfully connect, and port 8081 is refused.

Basically, I'd like to see consistency more than anything and figured the easiest way to do this was remove the ability to call .listen() multiple times.

[eduardbme]

Hi there.
I am new to contribution to nodejs and decided to start with 'good first contribution' label.
It looks like you discussed this problem, but I've compiled current master branch and the problem still exists and the problem still open..
So I wrote simple solution with _listenHasBeenCalled variable and I am wondering whether community still interesting in it.
Many thanks.

[Trott]

Removing good first contribution since we have a PR open for this: #13149