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());
}