Include listen errors in app.listen() callback

[wesleytodd]

I know that you can listen on the server instance, but it seems like this might be nicer to just proxy up the error event. Thoughts?

[dougwilson]

You can call app.listen multiple times, for example listening on multiple ports. How will a user know which server caused the error?

This is also pretty breaking, because no one has been adding error event listeners on app, and now, even if they added the appropriate listening to the returned server, their app will crash.

[dougwilson]

Also, if you were just adding this for the listening error, the .listen method accepts a callback so you can just handle the error there:

app.listen(3000, function (err) {
  // ... do stuff
})

[wesleytodd]

@dougwilson Ok, thats cool, it was just a one line change so I figured I would do it and see your thoughts. Your points make perfect sense, I guess I have just never used multiple listens (I use nginx for ssl term).

For the callback scenario, maybe I am doing something wrong, but the EADDRINUSE error causes an uncaught exception and crashes the server. Here is my code:

process.on('uncaughtException', function(err) {
    console.error('Uncaught: ', err);
});
app.listen(app.get('port'), app.get('hostname'), function(err) {
  if (err) {
    return console.error('Error starting server', err);
  }
  console.log('Listeneing');
});

And the output is:

$ node index.js
Uncaught:  { [Error: listen EADDRINUSE 127.0.0.1:3001]
  code: 'EADDRINUSE',
  errno: 'EADDRINUSE',
  syscall: 'listen',
  address: '127.0.0.1',
  port: 3001 }

This situation is the only reason I thought this might be a nicer api, since the best one is apparently not working correctly now.

[dougwilson]

For the callback scenario, maybe I am doing something wrong, but the EADDRINUSE error causes an uncaught exception and crashes the server.

Oops, I was not remembering correctly. The callback function is actually passed as the listening event listener within Node.js. Perhaps a patch that would make the callback given to Express's listen method callback on the first listening or error event may be desirable? The module ee-first should make this trivial :)

[wesleytodd]

Ignoring the fact that i didn't change the tests, what do you think of this: wesleytodd@a8bc849

Also ignore the tabs, I would fix those up before submitting. :)

[dougwilson]

In general, looks fine. I would skip manipulating the arguments object directly and just copy to an intermediate array. Otherwise, I'd accept this PR with this new style :)

[wesleytodd]

Ok, hows that?

[dougwilson]

Should probably use the same context as the listening event does: against ee instead of app.

[wesleytodd]

Good call

[wesleytodd]

Let me know if you want me to change anything else!!

[dougwilson]

Thanks, @wesleytodd , looks fine and targeted for 4.13 :)

[dougwilson]

Can this port number be made dynamic? Getting some race conditions where the Express suite is running multiples times on the same machine and they all want to use port 9999. Just have app listen randomly and have app2 listen to whatever port app is listening on should work.

[wesleytodd]

Yeah, I can make this change for sure. Let me know if you also want me to move this PR to the 5.0 branch and I will do it all at once.

[dougwilson]

Ok, cool, I got your changes all fixed up :) I found while testing that there are some issues with adding this in 4.x, namely that people are unconditionally assuming the callback means the server is up. I definitely love this convenience and will have it land in 5.0 instead \m/

[wesleytodd]

Sounds good!! let me know if there is anything else you need from me, but it looks like it is all good!

[wesleytodd]

If this is slated for 5, should I re-make this PR against the 5.0 branch? Or are you pulling in the change manually? If so I can close this out.

[wesleytodd]

Closed in favor of #3216