Ensuring threads running infinite loops are terminated before exiting the program

Question

As a hobbyist programmer, I gain knowledge where I can find it from google searches. I tend to write code that utilizes threads that have infinite loops using while loops, where the while condition slaves onto a BooleanProperty that when set false, causes the thread to interrupt the loop. I use a single class to keep track of the threads that are still looping and when they have all stopped, it can then exit the program.

A specific example of how I use a thread like this is an app I wrote that keeps my One Time Passwords for two-factor authentication on various web sites. Those passwords change every 30 seconds at second 0 of a minute change and at second 30 within a minute. Within each 30-second time frame, I do things to the label that is showing the password for an account such as fade the color to red during the last 10 seconds, then fade opacity in the last second and the first second, and there is simply no possible way to make those changes event-driven, so I use a looping thread to determine when those events need to happen.

However, it was brought to my attention that the way I am doing this might not be the best way, so I would like to share how I am doing this and see if anyone can suggest a better method.

I took a few Java classes when I got my CIS degree, but nothing covering advanced topics in Java. So I admit my knowledge is far from where I want it to be where wielding the Java language is concerned. I'd consider myself a hack programmer, but I would like my knowledge to be more complete than hack-level, but I don't know what I don't know so It's difficult to determine what to study given what I do know... you know?

Anyway, the way I have been typically handling this issue, is that I first start with a ThreadRegister class that is publicly static and it looks like this:

package ClassHelpers;

import javafx.beans.property.BooleanProperty;
import javafx.beans.property.SimpleBooleanProperty;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;

public class ThreadRegister {

    public static  BooleanProperty keepRunning  = new SimpleBooleanProperty(true);
    private static List<Long>      threadIDList = new ArrayList<>();
    private static long            threadID     = 0;

    public static long addThread() {
        threadID++;
        threadIDList.add(threadID);
        return threadID;
    }

    public static void removeThread(Long threadID) {
        threadIDList.remove(threadID);
    }

    public static void exitApp() {
        new Thread(() -> {
            keepRunning.setValue(false);
            while (threadIDList.size() > 0) {
                try {TimeUnit.MILLISECONDS.sleep(10);} catch (InterruptedException e) {e.printStackTrace();}
            }
            System.exit(0);
        }).start();
    }
}

The publicly static BooleanProperty is then bound to the same object type with the same name in any other class that has looping threads, and such a class would look like this:

package Tests;

import ClassHelpers.ThreadRegister;
import javafx.beans.property.BooleanProperty;
import javafx.beans.property.SimpleBooleanProperty;
import java.util.concurrent.TimeUnit;

public class MyClass {

    public MyClass() {
        this.keepRunning.bind(ThreadRegister.keepRunning);
    }

    private BooleanProperty keepRunning = new SimpleBooleanProperty();

    private void myThread() {
        new Thread(()->{
            long threadID = ThreadRegister.addThread();
            while (keepRunning.getValue().equals(true)) {
                //Do necessary processing here then
                //Sleep for a short time to keep CPU consumption low
                try {TimeUnit.MILLISECONDS.sleep(50);} catch (InterruptedException e) {e.printStackTrace();}
            }
            ThreadRegister.removeThread(threadID);
        }).start();
    }
}

I always make sure that any loops within a threads' main while loop also depend on that BooleanProperty with sleep values in every loop sitting right around 50 milliseconds to make sure that when the app is closed, the threads stop quickly. And last, I set the JavaFX Stage onCloseRequest value to the ThreadRegisters exitApp() method like this:

this.stage.setOnCloseRequest(e->ThreadRegister.exitApp());

Now, no matter how many of these threads I have running, when the stage is close requested, the ThreadRegister sets the BooleanProperty to false, then it waits for all of the threads to stop running before finally exiting the app.

Answer 1

Hello and welcome to code review! My main question is: How often do you write programs that have several threads running throughout the application lifecycle with identical exit condition? It is possible that this might negatively affect the way you design your applications and make you use practises that aren't quite optimal? For example, instead of starting and stopping a thread when needed, do you now have threads running forever and waiting for a specific start signal?

Java already has quite powerful tools for managing concurrency, such as executors and parallel streams. I think you might benefit from studying those before rolling your own.

Code

The ThreadRegister has one glaring bug: even though it deals with threads and concurrency, it is not thread safe itself. Access to threadIDList and threadID should be synchronized.

You should follow Java naming conventions. Mainly, packages should be in lower case.

By using property-classes from the javafx package, you have created a dependency from a fairly generic looking utility to a full blown desktop application framework. This limits the usability of your utility a lot.

Answer 2

As for a real review, see Torben's answer.

Apart from that: have a look at the method Thread.setDaemon(). Setting your worker threads to daemon would probably solve your problem without any further code. (See this question in SO https://stackoverflow.com/questions/2213340/what-is-a-daemon-thread-in-java)

If you want to perform additional parallel tasks in JavaFX specifically, have a look at Tasks and Workers. For things like transitioning a color over time, have a look at JavaFX's "animations" and "timeline animations".

Answer 3

@TorbenPutkonen's answer includes all the important facts about your code, I want just to add some minor thoughts. When you works with java threads and you have some shared variables check if there is a thread safe alternative already implemented in the java library. So instead of long and boolean use the thread safe alternatives AtomicLong and AtomicBoolean, same approach for the list that must be synchronized.