Starting and stopping a thread for a command handler

I built an application class that should be runnable as a Thread and it would be nice to hear opinions of Java developers to improve my coding style.

Main class.

package com.ozankurt;

public class Main {

    public static void main(String[] args) {

        Application application = new Application();

        application.start();

    }
}

Application class:

package com.ozankurt;

import com.ozankurt.utilities.Input;

public class Application implements Runnable {

    private CommandHandler commandHandler;

    private boolean running;

    private volatile Thread thread;
    private String threadName = "ApplicationThread";

    public Application() {

        Handler handler = new Handler(this);

        this.commandHandler = new CommandHandler(handler);

        this.running = true;
    }

    public void start() {

        if (thread != null) {
            System.out.println(threadName + "is already running.");

            if (!isRunning())
            {
                System.out.println("Resuming " + threadName + "...");
                setRunning(true);
            }
        } else {
            System.out.println("Starting " + threadName + "...");

            thread = new Thread(this, threadName);
            thread.start();

            System.out.println("Started " + threadName + ".");
        }
    }

    public void stop() {
        System.out.println("Halting " + threadName + "...");

        setRunning(false);

        System.out.println("Halted " + threadName + ".");
    }

    @Override
    public void run() {

        while (isRunning()) {
            String userInput = Input.readLine();

            commandHandler.handle(userInput);
        }
    }

    public boolean isRunning() {

        return running;
    }

    public void setRunning(boolean running) {

        this.running = running;
    }
}

I read this Java documentation about how to stop threads in Java without using stop method that has become deprecated bec

Your approach is works for a single start-stop cycle. Afterwards, things may go wrong. Let's look at the simple scenario first, though:

You should keep in mind that you want to halt the running threads as soon as possible whilst maintaining system functionality. That is, when I say stop, and there's no calculating or whatever going on, the system ought to stop. But in your case, it might not.

You see, what you have here,

while (isRunning()) {
        String userInput = Input.readLine();

        commandHandler.handle(userInput);
    }

Is either a spin loop or does not check the isRunning fast enough. Or so I think.

If there is no timeout for Input.readLine() and it does block until input is required, then stopping the application whilst it is waiting for input will lead to the application only actually shutting down AFTER the input has arrived and been handled. In case of a system shutdown, or programmed shutdown, this input may never arrive. The JVM will handle power situations for you, forcibly killing your thread, but if you were to use this as a library and another thread was told to shutdown the application your application thread wouldn't be shut down.

If there is no timeout for Input.readLine() and it does not block until input is required, then you've got a spin loop. When running, the application will check thousands of times whether you've got input, and then pass the non-existing input (maybe as empty string, maybe null), through your commandhandler. This is a waste of system resources.

By setting a timeout on the input reading, you can resolve this issue: By blocking for only 100ms (or so) for input, you can reduce the amount of checks to about 10 per second (which is easily handled by processors), whilst at the same time maintaining a fast response upon shutdown.

Secondly, your code is not threadsafe. There are certain cases which will cause things you do not want to happen.

If two threads call start() at the same time, then...

public void start() {

    if (thread != null) {
        System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            setRunning(true);
        }
    } else {
        System.out.println("Starting " + threadName + "...");

        thread = new Thread(this, threadName);
        thread.start();

        System.out.println("Started " + threadName + ".");
    }
}

Both threads start at the top of the function.

First line is this:

if (thread != null)

Well, thread is null in both cases. Thread 1 goes into the else statement, and so does thread 2.

else {
        System.out.println("Starting " + threadName + "...");

        thread = new Thread(this, threadName);
        thread.start();

        System.out.println("Started " + threadName + ".");
    }

No checks occur here, so both threads start the application. You now have 2 application threads running.

Now, there are a few more scenarios I could describe, but there are some structural flaws in the code which should be addressed before we go into that.

First, you keep track of your state like this:

public Application() {

    Handler handler = new Handler(this);

    this.commandHandler = new CommandHandler(handler);

    this.running = true;
}

In the constructor you set running to true. But the application isn't running yet! What you should do is set running to true in the start function. That way, whenever start is called, the result is that the application is running (either it was running before, or it was started).

Secondly, whenever a thread needs to be "resumed", you check like this:

System.out.println(threadName + "is already running.");

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            setRunning(true);
        }

That's not going to work, sadly. When you call stop, and set running to false, the thread can leave the while loop and terminate. A terminated thread cannot be restarted. So you must use a new thread!

But even this is not that easy - let's go with the naive approach first.

First, since we are going to be making threads both in the regular (no thread running) and resuming (thread was running but has terminated) functions of start, let's extract threadmaking to a separate function:

private void startApplicationThread(){
    thread = new Thread(this, threadName);
    setRunning(true);
    thread.start();
}

Next, add the startApplicationThread to the start function like so...

```
public void start() {

if (thread != null) {
System.out.println(threadName + "is already running.");

if (!isRunning())
{
System.out.println("Resuming " + threadName + "...");
startApplicationThread();
}
} else {
System.out.println("Starting " + threadName + "...");

thread = new

StackExchange Code Review Q#118217, answer score: 6