4
\$\begingroup\$

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 because it might throw some exceptions.

As far as I understand we shouldn't try to stop a thread and instead we should define a property corresponding to the state of the thread.

In my case I defined a boolean running in my Application class and as you can read from my code in the run method I basicly check if the application is running. Which mean that I actually never stop the thread, I only remove its functionality by using a while statement. Is this the correct approach explained in the Java documentation?

\$\endgroup\$
3
  • \$\begingroup\$ Please see What to do when someone answers. \$\endgroup\$ – 200_success Jan 29 '16 at 10:57
  • \$\begingroup\$ From what I saw by looking at the exception you posted - keep in mind, we don't do broken code here - you need to check if the line you got from Input.readLine is not null. \$\endgroup\$ – Pimgd Jan 29 '16 at 10:57
  • \$\begingroup\$ @Pimgd Yep. :) That did the trick. \$\endgroup\$ – Ari Nubar Boyacıoğlu Jan 29 '16 at 11:19
6
\$\begingroup\$

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 Thread(this, threadName);
        thread.start();

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

But, like this, we have another case of thread unsafe code: If thread 1 and thread 2 resume application via start at the same time, they will both reach startApplicationThread!

So let's fix that.

It's pretty easy to fix - just add synchronized. What synchronized does is say "only 1 thread can be in any method that is synchronized per instance at the same time". For now, this is good enough, but later you may want to use locking objects to prevent other possible coding errors.

public synchronized void start()
public synchronized void stop()

But it's still not threadsafe!

Let's say we have 1 thread running the application and 1 external thread. The external thread first calls stop. It goes through and running is now false. The application thread, however, is still busy with Input.readLine. That is to say, it's still waiting for input - as a result, it's still in the while loop and won't be going anywhere.

Quick overview of the situation:

Application thread is here:

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

        commandHandler.handle(userInput);
    }

External thread is here:

//external code
application.stop();

// <= externalthread
application.start();

And running is false.

Next, the external thread calls start. Because there is no other thread busy with start or stop, it can go into the method. It goes through the checks, thread != null is true, !isRunning() is also true, and starts a new thread (because the old one is "dead"). running is set to true again... and... oh snap.

Application threads:

    //<= applicationthread 2
    while (isRunning()) {
        String userInput = Input.readLine(); // <= applicationthread

        commandHandler.handle(userInput);
    }

External thread is here:

//external code
application.stop();

application.start();
// <= externalthread

And running is true, so application thread 2 will soon be moving into the while loop.

Now we've got 2 application threads again! Darn it.

So, before we start a new thread, we should make sure that it's actually dead. Now, whilst it is possible to save the running thread, it makes things hard, so lets discard the running thread - wait for it to die first.

Looking through the Javadoc of Thread, we can find Thread.join, which does this: "Waits for this thread to die." Great!

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

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

Now you get a compiler error because you need to catch InterruptedException - maybe you ought to deal with that in a cleaner way, but for now I'm going to ignore it.

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

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            try {
                thread.join();
            } catch (InterruptedException ie){
                //ignore
            }
            startApplicationThread();
        }
    }

Oh wait, now we could get InterruptedException whilst waiting. Not good. Let's just throw a while loop around it: (not the cleanest solution, but it'll work)

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

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            boolean stoppedRunningThread = false;
            while(!stoppedRunningThread){
                try {
                    thread.join();
                    stoppedRunningThread = true;
                } catch (InterruptedException ie){
                    //ignore
                }
            }
            startApplicationThread();
        }
    }

Might even make it do-while, since we have to do this at least once anyway.

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

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            boolean stoppedRunningThread = false;
            do {
                try {
                    thread.join();
                    stoppedRunningThread = true;
                } catch (InterruptedException ie){
                    //ignore
                }
            } while(!stoppedRunningThread);
            startApplicationThread();
        }
    }

So, now we are done, right?

Well... not quite. You see, what would happen if the thread that wants to restart the application is the application thread?

Well, we'd go through start, and wait for the currently running thread to... die... Yeah, that's not going to work! In that case, we can use Thread.currentThread() to compare! If we find that the thread that is restarting the application is the application thread, then we don't need to wait for it to die, we can just set running to true again, because we know it is still in the while loop and won't go out of the while loop whilst we are setting the variable.

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

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            if (thread != Thread.currentThread())
            {
                boolean stoppedRunningThread = false;
                do {
                    try {
                        thread.join();
                        stoppedRunningThread = true;
                    } catch (InterruptedException ie){
                        //ignore
                    }
                } while (!stoppedRunningThread);
                startApplicationThread();
            } else {
               setRunning(true);
            }
        }
    }

So now, finally, everything is threadsafe, yes?

Well no. You see, this method here is public:

public void setRunning(boolean running) {

    this.running = running;
}

And that ruins pretty much everything. You can kill the application at any time by just calling setRunning(false). What's worse, an application that's been stopped and been told to setRunning(true) can't be restarted! So you should make it private, and access it via start and stop.

private void setRunning(boolean running) {
    this.running = running;
}

So, total overview of how the class is looking right now:

package com.ozankurt;

import com.ozankurt.utilities.Input;

public class Application implements Runnable {

    private CommandHandler commandHandler;

    private boolean running = false;

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

    public Application() {

        Handler handler = new Handler(this);

        this.commandHandler = new CommandHandler(handler);
    }

    public synchronized void start() {

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

            if (!isRunning())
            {
                System.out.println("Resuming " + threadName + "...");
                if (thread != Thread.currentThread())
                {
                    boolean stoppedRunningThread = false;
                    do {
                        try {
                            thread.join();
                            stoppedRunningThread = true;
                        } catch (InterruptedException ie){
                            //ignore
                        }
                    } while (!stoppedRunningThread);
                    startApplicationThread();
                } else {
                   setRunning(true);
                }
            }
        } else {
            System.out.println("Starting " + threadName + "...");

            startApplicationThread();

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

    public synchronized 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;
    }

    private void setRunning(boolean running) {

        this.running = running;
    }
}

Okay.

So is it threadsafe NOW?! Hmmmm... well... no.

Here's a new situation - an external thread stops the application, and then starts it again, but meanwhile the application thread ALSO wants to stop the application.

Basically like so:

//external code
application.stop();

application.start();

//... inside application.start()...

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

        if (!isRunning())
        {
            System.out.println("Resuming " + threadName + "...");
            if (thread != Thread.currentThread())
            {
                boolean stoppedRunningThread = false;//<= external thread
                do {
                    try {
                        thread.join();
                        stoppedRunningThread = true;
                    } catch (InterruptedException ie){
                        //ignore
                    }
                } while (!stoppedRunningThread);
                startApplicationThread();
            } else {
               setRunning(true);
            }
        }
    }

And the application thread is here:

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

            commandHandler.handle(userInput);//<= applicationthread
        }

Which is going to stop the application because the user typed "quit".

What happens is that because the external thread is in the start function, the application thread cannot enter stop, and as a result, it has to wait. But it's still alive. So the external thread, waiting in thread.join for the applicationthread to die, will have to wait forever.

A deadlock again!

Resolving this would be hard, but possible:

  • You could make the quit command break out of the while loop instead of calling stop
  • You could forcibly kill the thread after a bit of time
  • You could ignore the issue, how likely is it that this happens?
  • You could rewrite the locking to use a lock object, so that you can handle this in the stop method
  • You could make an application a single-use object, instantiating a new Application instead, making the whole restart-locking moot and everything a whole lot easier
  • You could make "quit" set a flag in the applicationthread, so that the application thread knows that it should stop the while loop even if running is true because the thread has quit...
  • Use "InterruptedException" to check if applicationthread is blocking you with a smart locking system
  • Use Locking libraries to be able to conditionally gain a lock before quitting, if you cannot gain a lock, and running is false and thread is not currentthread, then exit function

A whole list of options.

Here's mine:

        while (isRunning() && thread == Thread.currentThread()) {
            String userInput = Input.readLine();

            commandHandler.handle(userInput);
        }

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

        setRunning(false);
        thread = null;

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

This should take care of most of the issues with minimum effort. The rest, I opt to ignore because I think them too rare to occur.

Alternatively, I'd rewrite my application flow so that I don't get into all sorts of crazy start-stop-start-start-stop situations. I'd prefer to change the flow to use Application only once per instance - a simple lifecycle of "start, stop, remain dead".

\$\endgroup\$
5
  • \$\begingroup\$ Incredible! I totally missed the part you told about the line if (thread != null). When it comes to the first part about blocking the application, Input.readLine() blocks the app from going to the commandHandler.handle() line. I didn't totally understand the last sentence. Do you mean that: "If I try to stop the ApplicationThread from another thread the ApplicationThread will not stop because it's being blocked by the Input.readLine()". \$\endgroup\$ – Ari Nubar Boyacıoğlu Jan 29 '16 at 10:06
  • 1
    \$\begingroup\$ @AriNubarBoyacıoğlu I've added further explanation \$\endgroup\$ – Pimgd Jan 29 '16 at 10:22
  • \$\begingroup\$ One more question is now up there, I also added some Command classes and the whole CommandHandler. Though you might be interested in checking them out. :) \$\endgroup\$ – Ari Nubar Boyacıoğlu Jan 29 '16 at 10:56
  • \$\begingroup\$ @AriNubarBoyacıoğlu recommend you fix the exception you are getting and post a follow-up question with the rest of the code =) \$\endgroup\$ – Pimgd Jan 29 '16 at 10:58
  • \$\begingroup\$ :D Yeah, makes more sense. \$\endgroup\$ – Ari Nubar Boyacıoğlu Jan 29 '16 at 11:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.