CODEWITHSUNDEEP
javamultithreadingthreadpoolexecutorservice
alf
alf

Reputation: 43

Best practice for interrupting threads that take longer than a threshold

I am using the Java ExecutorService framework to submit callable tasks for execution. These tasks communicate with a web service and a web service timeout of 5 mins is applied. However I've seen that in some cases the timeout is being ignored and thread 'hangs' on an API call - hence, I want to cancel all the tasks that take longer than say, 5 mins.

Currently, I have a list of futures and I iterate through them and call future.get until all tasks are complete. Now, I've seen that the future.get overloaded method takes a timeout and throws a timeout when the task doesnt complete in that window. So I thought of an approach where I do a future.get() with timeout and in case of TimeoutException I do a future.cancel(true) to make sure that this task is interrupted.

My main questions
1. Is the get with a timeout the best way to solve this issue?
2. Is there the possibility that I'm waiting with the get call on a task that hasnt yet been placed on the thread pool(isnt an active worker). In that case I may be terminating a thread that, when it starts may actually complete within the required time limit?

Any suggestions would be deeply appreciated.

Upvotes: 4

Views: 2015

Answers (4)

Amol Sonawane
Amol Sonawane

Reputation: 1130

  1. Is the get with a timeout the best way to solve this issue?

    • This will not suffice. For instance, if your task is not designed to response to interruption, it will keep on running or be just blocked

  2. Is there the possibility that I'm waiting with the get call on a task that hasnt yet been placed on the thread pool(isnt an active worker). In that case I may be terminating a thread that, when it starts may actually complete within the required time limit?

    • Yes, You might end up cancelling as task which is never scheduled to run if your thread-pool is not configured properly

Following code snippet could be one of the way you can make your task responsive to interruption when your task contains Non-interruptible Blocking. Also it does not cancel the task which are not scheduled to run. The idea here is to override interrupt method and close running tasks by say closing sockets, database connections etc. This code is not perfect and you need to make changes as per requirements, handle exceptions etc.

class LongRunningTask extends Thread {
private  Socket socket; 
private volatile AtomicBoolean atomicBoolean;


public LongRunningTask() {
    atomicBoolean = new AtomicBoolean(false);
}

@Override
public void interrupt() {
    try {
        //clean up any resources, close connections etc.
        socket.close();
    } catch(Throwable e) {
    } finally {
        atomicBoolean.compareAndSet(true, false);
        //set the interupt status of executing thread.
        super.interrupt();
    }
}

public boolean isRunning() {
    return atomicBoolean.get();
}

@Override
public void run() {
    atomicBoolean.compareAndSet(false, true);
    //any long running task that might hang..for instance
    try {
        socket  = new Socket("0.0.0.0", 5000);
        socket.getInputStream().read();
    } catch (UnknownHostException e) {
    } catch (IOException e) {
    } finally {

    }
}
}
//your task caller thread
//map of futures and tasks 
    Map<Future, LongRunningTask> map = new HashMap<Future, LongRunningTask>();
    ArrayList<Future> list = new ArrayList<Future>();
    int noOfSubmittedTasks = 0;

    for(int i = 0; i < 6; i++) {
        LongRunningTask task = new LongRunningTask();
        Future f = execService.submit(task);
        map.put(f, task);
        list.add(f);
        noOfSubmittedTasks++;
    }

    while(noOfSubmittedTasks > 0) {
        for(int i=0;i < list.size();i++) {
            Future f = list.get(i);
            LongRunningTask task = map.get(f);
            if (task.isRunning()) {
                /*
                 * This ensures that you process only those tasks which are run once
                 */
                try {
                    f.get(5, TimeUnit.MINUTES);
                    noOfSubmittedTasks--;
                } catch (InterruptedException e) {
                } catch (ExecutionException e) {
                } catch (TimeoutException e) {
                                            //this will call the overridden interrupt method
                    f.cancel(true);
                    noOfSubmittedTasks--;
                }
            }

        }
    }
    execService.shutdown();

Upvotes: 2

Debojit Saikia
Debojit Saikia

Reputation: 10632

You can of course cancel a Task by using

task.cancel(true)

It is perfectly legal. But this will interrupt the thread if it is "RUNNING".

If the thread is waiting to acquire an intrinsic lock then the "interruption" request has no effect other than setting the thread's interrupted status. In this case you cannot do anything to stop it. For the interruption to happen, the thread should come out from the "blocked" state by acquiring the lock it was waiting for (which may take more than 5 mins). This is a limitation of using "intrinsic locking".

However you can use explicit lock classes to solve this problem. You can use "lockInterruptibly" method of the "Lock" interface to achieve this. "lockInterruptibly" will allow the thread to try to acquire a lock while remaining responsive to the interruption. Here is a small example to achieve that:

public void workWithExplicitLock()throws InterruptedException{
Lock lock = new ReentrantLock();
lock.lockInterruptibly()();
try {
// work with shared object state
} finally {
lock.unlock();
}

}

Upvotes: 0

Trying
Trying

Reputation: 14278

The approach that you are talking about is ok. But most importantly before setting a threshold on the timeout you need to know what is the perfect value of thread pool size and timiout for your environment. Do a stress testing which will reveal whether the no of worker threads that you configured as part of Threadpool is fine or not. And this may even reduce the timeout value. So this test is most important i feel.

Timeout on get is perfectly fine but you should add to cancel the task if it throws TimeoutException. And if you do the above test properly and set your thread pool size and timeout value to ideal than you may not even need to cancel tasks externally (but you can have this as backup). And yes sometimes in canceling a task you may end up canceling a task which is not yet picked up by the Executor.

Upvotes: 0

Narendra Pathai
Narendra Pathai

Reputation: 41945

Is the get with a timeout the best way to solve this issue?

Yes it is perfectly fine to get(timeout) on a Future object, if the task that the future points to is already executed it will return immediately. If the task is yet to be executed or is being executed then it will wait until timeout and is a good practice.

Is there the possibility that I'm waiting with the get call on a task that hasnt yet been placed on the thread pool(isnt an active worker)

You get Future object only when you place a task on the thread pool so it is not possible to call get() on a task without placing it on thread pool. Yes there is a possibility that the task has not yet been taken by a free worker.

Upvotes: 0

Related Questions