Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot catch EADDRINUSE from server.start() #2561

Closed
mik01aj opened this issue May 26, 2015 · 9 comments · Fixed by #2570
Closed

Cannot catch EADDRINUSE from server.start() #2561

mik01aj opened this issue May 26, 2015 · 9 comments · Fixed by #2570
Labels
bug Bug or defect

Comments

@mik01aj
Copy link

mik01aj commented May 26, 2015

When I try to call server.start(callback), and the port is in use, the script crashes with a message:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: listen EADDRINUSE
    at errnoException (net.js:904:11)
    at Server._listen2 (net.js:1042:14)
    at listen (net.js:1064:10)
    at net.js:1146:9
    at dns.js:72:18
    at process._tickCallback (node.js:419:13)

I would expect the callback to be called instead (with the error, of course).

@hueniverse
Copy link
Contributor

We don't wrap everything in try..catch code. Only place throws are handled is in userland code called as part of request processing so that we can return a 500 page.

@hueniverse hueniverse added the non issue Issue is not a problem or requires changes label May 27, 2015
@hueniverse hueniverse self-assigned this May 27, 2015
@mik01aj
Copy link
Author

mik01aj commented May 27, 2015

Well, if you don't handle errors well, then I doubt whether it's safe to use hapi in production.

EADDRINUSE is an error to be expected in the listen function and handling it is as simple as adding the callback to the listen call. The documentation even provides a detailed example on how to do this.

@ghost
Copy link

ghost commented May 27, 2015

Can't you just check the exit status ($?) in your init script?

@mik01aj
Copy link
Author

mik01aj commented May 27, 2015

@jzorn I use forever to restart the API, to be on the safe side.

But still, letting the application crash is a bug to me. You don't know anything about the application's open database transactions, half-written files and other ongoing operations. Hapi is just a library and it should not crash the process without an important reason. And if in your opinion it's still ok, then the documentation should tell Hapi users something like:

if the connection port happens in use, your whole application will crash and you have no way to handle it.

@edimoldovan
Copy link

I am not sure I follow the reasoning behind the need to handle the EADDRINUSE at startup. It apears only at startup, right? At least I only saw it there. If this is the case, then you basically don’t have any database connections which are in use yet, because nobody could connect to the app due it failing to start.
What am I missing here?

@ghost
Copy link

ghost commented May 27, 2015

Can't you use forever's -m switch and use $? I also don't think that the application crashes when it's throwing the EADDRINUSE — it refuses to start, right? That's pretty much exactly what every other service will do, e.g. superstatic.

@hueniverse
Copy link
Contributor

@mik01aj If you rely on EADDRINUSE errors in production code, you have bigger problems. I have never seen an app that tries to resolve that in production. You production setup must ensure a proper port is configured and that no multiple instances are running at the same time. This is clearly not the framework's job.

In general, we cannot possibly handle every exception every function might throw. Node has made a choice to throw in async methods when the basic inputs are wrong, even when a callback is provided.

I am happy to take a pull request that states simply that the method may throw and give this as an example.

@kanongil
Copy link
Contributor

EADDRINUSE is an error to be expected in the listen function and handling it is as simple as adding the callback to the listen call. The documentation even provides a detailed example on how to do this.

This is wrong. The callback is never called on errors. The only way to catch the error, is to add an error listener to the object listen is called on.

@hueniverse I agree on your point regarding relying on EADDRINUSE errors, however I am not sure I agree on the hands off approach you are pursuing in this case.

This is not a matter of adding try..catch but rather of respecting the api of the listen() call, which will either emit a listening, or an error event on the listener. You handle one case but ignore the other, which triggers the OMG no error listener! throw behavior.

Note that this issue will also trigger on this (with another error):

var server = new Hapi.Server();
server.connection({ host: 'does.not.resolve' });
server.start(function (err) {});

@hueniverse hueniverse reopened this May 27, 2015
@hueniverse
Copy link
Contributor

@kanongil fair enough. I'll take a PR.

@hueniverse hueniverse added bug Bug or defect help wanted and removed non issue Issue is not a problem or requires changes labels May 27, 2015
@hueniverse hueniverse removed their assignment May 27, 2015
arb added a commit to arb/hapi that referenced this issue May 28, 2015
arb added a commit to arb/hapi that referenced this issue May 28, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants