is it safe to store threads in a ConcurrentMap?

Question

I am building a backend service whereby a REST call to my service creates a new thread. The thread waits for another REST call if it does not receive anything by say 5 minutes the thread will die. To keep track of all the threads I have a collection that keeps track of all the currently running threads so that when the REST call finally comes in such as a user accepting or declining an action, I can then identify that thread using the userID. If its declined we will just remove that thread from the collection if its accepted the thread can carry on doing the next action. i have implemented this using a ConcurrentMap to avoid concurrency issues.

Since this is my first time working with threads I want to make sure that I am not overlooking any issues that may arise. Please have a look at my code and tell me if I could do it better or if there's any flaws.

public class UserAction extends Thread {

    int     userID;
    boolean isAccepted  = false;
    boolean isDeclined  = false;
    long    timeNow     = System.currentTimeMillis();
    long    timeElapsed = timeNow + 50000;

    public UserAction(int userID) {
        this.userID = userID;
    }

    public void declineJob() {
        this.isDeclined = true;
    }

    public void acceptJob() {
        this.isAccepted = true;
    }

    public boolean waitForApproval(){   
        while (System.currentTimeMillis() < timeElapsed){
        System.out.println("waiting for approval");
        if (isAccepted) {
            return true;
        } else if (declined) {
            return false;
        }

    }
        return isAccepted;
    }

    @Override
    public void run() {
        if (!waitForApproval) {
            // mustve timed out or user declined so remove from list and return thread immediately
            tCollection.remove(userID);
            // end the thread here
            return;
        }
        // mustve been accepted so continue working
    }
}

public class Controller {

    public static ConcurrentHashMap<Integer, Thread> tCollection = new ConcurrentHashMap<>();

    public static void main(String[] args) {
        int barberID1 = 1;
        int barberID2 = 2;

        tCollection.put(barberID1, new UserAction(barberID1));
        tCollection.put(barberID2, new UserAction(barberID2));

        tCollection.get(barberID1).start();
        tCollection.get(barberID2).start();

        Thread.sleep(1000);
        // simulate REST call accepting/declining job after 1 second. Usually this would be in a spring mvc RESTcontroller in a different class.
        tCollection.get(barberID1).acceptJob();
        tCollection.get(barberID2).declineJob();

    }
}

Answer 1

You don't need (explicit) threads for this. Just a shared pool of task objects that are created on the first rest call.

When the second rest call comes, you already have a thread to use (the one that's handling the rest call). You just need to retrieve the task object according to the user id. You also need to get rid of expired tasks, which can be done with for example a DelayQueue.

Pseudocode:

public void rest1(User u) {
    UserTask ut = new UserTask(u);
    pool.put(u.getId(), ut);
    delayPool.put(ut);  // Assuming UserTask implements Delayed with a 5 minute delay
}

public void rest2(User u, Action a) {
    UserTask ut = pool.get(u.getId());
    if(!a.isAccepted() || ut == null)
        pool.remove(u.getId());
    else
        process(ut);

    // Clean up the pool from any expired tasks, can also be done in the beginning
    // of the method, if you want to make sure that expired actions aren't performed
    while((UserTask u = delayPool.poll()) != null)
        pool.remove(u.getId());
}

Answer 2

There's a synchronization issue that you should make your flags isAccepted and isDeclined of class AtomicBoolean. A critical concept is that you need to take steps to make sure changes to memory in one thread are communicated to other threads that need that data. They're called memory fences and they often occur implicitly between synchronization calls.

The idea of a (simple) Von Neumann architecture with a 'central memory' is false for most modern machines and you need to know data is being shared between caches/threads correctly.

Also as others suggest, creating a thread for each task is a poor model. It scales badly and leaves your application vulnerable to keeling over if too many tasks are submitted. There is some limit to memory so you can only have so many pending tasks at a time but the ceiling for threads will be much lower. That will be made all the worse because you're spin waiting. Spin waiting puts a thread into a loop waiting for a condition. A better model would wait on a ConditionVariable so threads not doing anything (other than waiting) could be suspended by the operating system until notified that the thing they're waiting for is (or may be) ready.

There are often significant overheads in time and resources to creating and destroying threads. Given that most platforms can be simultaneously only executing a relatively small number of threads creating lots of 'expensive' threads to have them spend most of their time swapped out (suspended) doing nothing is very inefficient.

The right model launches a pool of a fixed number of threads (or relatively fixed number) and places tasks in a shared queue that the threads 'take' work from and process.

That model is known generically as a "Thread Pool". The entry level implementation you should look at is ThreadPoolExecutor: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html