Adding ability to control whether the httpserver is a daemon thread - #300

why not always setDaemon?

It does not matter in this case. I just looked through the code of DefaultThreadFactory and they use similar code:

  if (t.isDaemon())
    t.setDaemon(false);

Will change to setDaemon since it looks simpler.

Comments should be full sentences.

The class is HTTPServer

This message related to exception on starting com.sun.net.httpserver.HttpServer, but if you insist i will change.

Is this really necessary? We've just re-throwing the exception.

Otherwise i have to implement internal class which should implement Runnable and store exceptions in the instance fields to re-throw them later in the main thread:

class StartTask implements Runnable {
    private volatile RuntimeException exception;
    private volatile Error error;

    @Override
    public void run() {
        try {
            server.start();
        } catch (RuntimeException e) {
            exception = e;
        } catch (Error e) {
            error = e;
        }
    }

    void rethrow() {
        if (exception != null) throw exception;
        if (error != null) throw error;
    }
}
StartTask task = new StartTask();
Thread thread = CustomThreadFactory.defaultThreadFactory(daemon).newThread(task);
thread.start();
try {
    thread.join();
} catch (InterruptedException e) {
    //should not happen. but propagate interrupted flag just in case.
    Thread.currentThread().interrupt();
}
task.rethrow();

i decided to use tool that JDK provides for this – FutureTask

I don't see why the current implementation has to do so much exception handling, I would expect it to have to do none.

This also all runs in the main thread.

In the current implementation you just call server.start() in main thread. It can complete successfully or with Error or RuntimeException. In the latest case the HTTPServer will not be created and exception will be propagated to HTTPServer constructor caller. Now put server.start() into a separate thread and i want to propagate potential Error/RuntimeException to the HTTPServer again.

I must be missing something here. All I see is rethrowing exceptions in the same thread. Thus this seems quite a bit more complicated than it needs to be.

Ok. Will try to explain. RuntimeException or Error may happen in a thread that i launch in

DaemonThreadFactory.defaultThreadFactory(daemon).newThread(startTask).start();

The call startTask.get() will block until task is completed. Since it is blocking call it can be interrupted and throw InterruptedException. InterruptedException is a checked exception and should be handled explicitly. The same with an ExecutionException, which is in signature of a Future.get and I have to handle it as well. Since I can not extend signature of the HTTPServer constructors via new checked exception, I have to handle it here and the best way to do not swallow an error is to re-throw as RuntimeException/Error.

@brian-brazil any thoughts?

You can simplify by just rethrowing this. It's probably a port already in use.

Do you mean rethrowing ExecutionException? In JDK it is declared as

public class ExecutionException extends Exception

It is a checked exception and therefore to rethrow it I have to change signature of start to

private void start(boolean daemon) throws ExecutionException {

and then I have to change signature of every constructor of HTTPServer and add the same ExecutionException in it. It will break backward compatibility with all existing clients of HTTPServer class.
We know, that startTask.get(); executes server.start(); which can throw RuntimeException and Errors only, and the best place to convert ExecutionException into the cause RuntimeException or Error is in this place rather then let clients of HTTPServer deal with it.

You could rethrow it as a RuntimeException

That is exactly what I’m doing here, with a small optimization. This is a standard practice in Java world to analyze cause of ExecutionException and rethrow it. But if you prefer to wrap a wrapper of runtime exception into yet another runtime exception I can do that to push this fix forward, since it is critical to update to the new version of jmx_exporter

@brian-brazil changed per review.