Javalin: Javalin.start() should throw if startup fails

[mvysny]

Actual behavior (the bug)
I have another server occupying the port 8080. The Javalin.start() function completes successfully, but Javalin is not running.

Expected behavior
Since start() is synchronous, I expect the start() function to throw an exception if Javalin fails to start (e.g. on BindException).

To Reproduce

1. run netcat -l 8080
2. start Javalin on port 8080: Javalin.create().start(8080).

[tipsy]

Hm. So we do

} catch (Exception e) {
    log.error("Failed to start Javalin", e);
    if (e instanceof BindException && e.getMessage() != null) {
        if (e.getMessage().toLowerCase().contains("in use")) {
            log.error("Port already in use. Make sure no other process is using port " + port + " and try again.");
        } else if (e.getMessage().toLowerCase().contains("permission denied")) {
            log.error("Port 1-1023 require elevated privileges (process must be started by admin).");
        }
    }
    eventManager.fireEvent(JavalinEvent.SERVER_START_FAILED);
}

You want a throw new RuntimeException(e); at the end?

[tipsy]

What about:

} catch (Exception e) {
    log.error("Failed to start Javalin");
    eventManager.fireEvent(JavalinEvent.SERVER_START_FAILED);
    if (e.getMessage() != null && e.getMessage().contains("Failed to bind to")) {
        throw new RuntimeException("Port already in use. Make sure no other process is using port " + port + " and try again.", e);
    } else if (e.getMessage() != null && e.getMessage().contains("Permission denied")) {
        throw new RuntimeException("Port 1-1023 require elevated privileges (process must be started by admin).");
    }
    throw new RuntimeException(e);
}

[tipsy]

Fixed in https://github.com/tipsy/javalin/commit/85ea99ad6285c9f09b402b822e719713a076a25b, which also fixed a hacky test.

[mvysny]

Wow, you're really fast! Awesome work. I thought about throwing exceptions straight away instead of logging, but you beat me to it :-)

I'd only suggest to add cause to the second throw - the , e part:

throw new RuntimeException("Port 1-1023 require elevated privileges (process must be started by admin).", e);

[tipsy]

I'd only suggest to add cause to the second throw - the , e part:

Thanks, missed that one. Fixed in https://github.com/tipsy/javalin/commit/10ebe08b8c10a390baeb0ad2cd607cc285eb1d91.

Wow, you're really fast! Awesome work.

There's been a lot of traffic the past few days because of the article in Java Magazine, so I'm trying to respond as quickly as possible :)

[tipsy]

This has been released as part of 2.8.0 now: https://javalin.io/news/2019/03/26/javalin-2.8.0-released.html

[jhult]

The Javalin constructor initializes jettyServer (via JettyServerUtil.defaultServer() or otherwise).

@tipsy, doesn't this need to get killed or stopped when it fails to start? Otherwise, you get a phantom thread that keeps running.

[tipsy]

@jhult I'm guessing most people won't let their program continue running if startup fails, but it could be good to call stop() just in case?

Could you submit a PR?