Spark: Server shuts down when given port is in use

[Prototype1]

Hello,

we have a little problem with this code (JettySparkServer in method ignite()):

} catch (Exception e) {
logger.error("ignite failed", e);
System.exit(100); // NOSONAR
}

System.exit is actually pretty bad here. We have no chance in stopping the server from shutting down.

In our application we start a webserver and some other server. It is perfectly fine for us when the given port of the webserver is already in use but System.exit shuts down the whole jvm and thus our other server also.

Would be nice when there was some workaround (like an exception handler which is configurable outside of spark)

Thanks in advance!

Cheers,

[arun-y]

Even I am facing exactly the same problem, in our application its perfectly fine if Spark doesn't ignite due to port conflict. But the main application should continue running. However due to System.exit(); the primary application is killed. IMHO System.exit should be optional, at least in embedded mode.

[Prototype1]

Hi,

Its pretty easy to patch the class file to remove the problematic code.
What I did:
Copy the source code of the class JettySparkServer to a new File. Remove System.exit and compile it. Open Spark jar file and replace JettySparkServer class with your custom class.

It's an ugly workaround but works for us. The change was quite simply and didnt take me more than 15min.

[arun-y]

Thanks @Prototype1, I too thought of changing the source and recompile for our internal purpose. But was wondering if this behavior can be fixed in upstream too?

[Prototype1]

My idea (sadly no time to implement yet) to have a default handler which exits the VM and is mutable. The handler is in fact a simply Consumer<Exception>

[arun-y]

That's a good idea, so you mean users can supply custom handler/consumer which can choose not to exit VM in situation as ours?

[Prototype1]

Since I opened the issue the package structure changed quite a bit.

I would suggest having a private Consumer<Exception> fatalErrorHandler = (ex) -> { logger.error(ex); System.exit(100); }
In Class EmbeddedJettyServer

The fatalErrorHandler gets called everywhere where System.exit() shows up and has a simple setter to overwrite the default behavior.

[arun-y]

Seems perfect to me. If committer agrees we can provide PR. I can spare some time for this, if needed. :)

[Prototype1]

Well our program works with spark now and I am at vocation the next 2 weeks and wont have time to do it.
The hard part is to find a clean way to expose the functionality to the user. Didnt look too much into it but it seems you need to change a few interfaces.

Feel free to provide a PR. :+1: Would be cool!

[arun-y]

Alright, I am off till this weekend. Will try to do after that.

[arun-y]

@Prototype1 - You are right hard part is to expose the functionality to the user. I was wondering how about using some program argument or configuration to decide exit on exception vs. throwing exception to the caller? That may avoid changing interfaces.

[Prototype1]

Hi @arunwizz

passing a argument at start time works is however inflexible (I dont mind).

My idea is we delegate the functionality to the Service.
So https://github.com/perwendel/spark/blob/master/src/main/java/spark/Service.java

We add a method boolean onFatalException(Consumer<> handler) and return true if the given Server (EmbeddedJettyServer in our case) supports the functionality to set a fatalExceptionHandler.

Something like this:
boolean onFatalException(Consumer<> handler){ if(embeddedServer instanceof EmbeddedJettyServer){ embeddedServer.onFatalExcpetion(handler); return true; }else{ logger.log("embeddedServer doesnt support this feature"); return false; }

In https://github.com/perwendel/spark/blob/master/src/main/java/spark/Spark.java we simple have the same static method wich calls the service.

The cleanest way would probably be to add a default method to the interface https://github.com/perwendel/spark/blob/master/src/main/java/spark/embeddedserver/EmbeddedServer.java

configureFatalExceptionHandling() which returns false when not implemented.

Cheers,

Moritz

[arun-y]

Hi @Prototype1

You are right. Above approach is more flexible. I have one additional idea though, how about wrapping code inside Service.init() with try-catch block. We can define Service.fatalErrorHandler with default implementation. And allowing users to override Service.fatalErrorHandler with custom consumer.

This way EmbeddedServer and its underlying implementation should just throw all exception to the caller init() function of Service.java.

So something like this in Service.java

// default exception handler, same as what you suggested inside
// EmbeddedJettyServer.java,
private Consumer<Exception> exceptionHandler = (ex) -> { LOG.error(ex.getMessage()); System.exit(100); };

// we can even call this configureFatalExceptionHandling()
public void onFatalException(Consumer<Exception> handler) { this.exceptionHandler = handler; }

public synchronized void init() { try { //existing code which invokes EmbeddedServer.ignite(); //... } catch (Exception e) { exceptionHandler.accept(e); } }

And as you suggested we can have same static function "onFatalException" in Spark.java to allow users to override the behavior.

//we can even call this configureFatalExceptionHandling() public static void onFatalException(Consumer<Exception> handler) { getInstance().onFatalException(handler); }

After this we can remove try/catch from EmbeddedJettyServer.ignite() method and may add throws Exception clause in EmbeddedServer.ignite() method.

Does this make any sense?

Regards,
Arun

[Prototype1]

Hi @arunwizz
this would work too but only catches errors during server start up or?
Cheers,

[arun-y]

Yes @Prototype1 only during startup, I am under assumption that any exception during routing will be handled with existing exception mapping mechanism. I am not too verse with complete spark framework, so excuse me if I have missed out anything :)

Regards

[Prototype1]

Well I am not too much into spark either. But i guess your solution is simpler (didnt found any System.exit beside the start of the server, so handling ignit only is enough).

Cheers,

Moritz

[arun-y]

Appreciate your guidance @Prototype1 , I am not sure how this community works. I will probably incorporate this change in my fork, generally the authors/committers has to approve any change before raising PR. There are already 47 pending PRs :)

Regards
Arun

[Prototype1]

No Problem :) Always cool to help.

[tipsy]

There are already 47 pending PRs :)

Just 37 now :smirk:
This seems like a minor change, so please create a PR, and we'll have a look at it soon™.

[arun-y]

Thanks @tipsy

Just 37 now :smirk:

That's pretty quick!!

By the way what is ™ doing after soon :smile: ?

[tipsy]

I just dismissed most of them, so don't be too impressed.
That soon is more of a eventually :smile:

[PaulBGD]

Turns out that rethinkdb already listened on the port I was using (8080) 😌 I thought I was going crazy that a random 100 exit code kept popping up.

[tipsy]

You can now override the default behavior like this:
initExceptionHandler((e) -> System.out.println("Uh-oh"));