2

In a web server i wrote, each request invokes a list of actions. Some of these actions aren't as critical as others, so I would like to run them in a background thread.

Also, since they aren't that important I don't care if one of them fails seldomly, and I don't want them to take up a thread forever, so other threads would be available to process the next batch.

So, I would like to have a thread pool (e.g.: 10 threads) and hand out a thread to each background task like this. Limit each thread to 1 second, and if it doesn't finish by that time, just kill it, and be available for the next task to come in.

How would I go about doing this ?

So far, this is what I have :

public class AsyncCodeRunner {

    private static final ExecutorService executor = Executors.newFixedThreadPool(10);

    public void Run(Callable<Void> callableCode, int timeout) {

        final int threadTimeout = 10;
        Future<Void> callableFuture = executor.submit(callableCode);

        try {
            callableFuture.get(threadTimeout, TimeUnit.SECONDS);
        } catch (Exception e) {
            logger.Info("Thread was timed out", e);
        }
    }
}

And I want to use this class like this :

public void processRequest(RequestObject request) {

    // do some important processing

    // throw some less important processing to background thread
    (new AsyncCodeRunner()).Run(new Callable<Void> () {
        @Override
        public Void call() throws Exception {
            // do something...
            return null;
        }
    }, 1); // 1 second timeout

    // return result (without waiting for background task)
    return;

}

Will this work like I want it to ? Or how should I change it so it would ?
And what happens if I call Run() but there are no available threads in the threadpool to hand out ?

Machavity
  • 30,841
  • 27
  • 92
  • 100
gillyb
  • 8,760
  • 8
  • 53
  • 80
  • While it is possible to do it, you're going to generate a lot of cache thrashing with this method... Does it really have to be one miserable second? – fge Jan 23 '15 at 10:41
  • I suspect this should be a CodeReview question. – OldCurmudgeon Jan 23 '15 at 10:45
  • @fge - care to explain what you mean by cache threashing ? and how I could change this by changing the timeout ? – gillyb Jan 23 '15 at 11:03

2 Answers2

2

I think your primary problem with this rather elegant idea is that you are only timing out on the get of the Future, you are not actually aborting the process once it times out, you are just giving up waiting for it. The issue becomes even more complex when you realise that you may even time out when the process hasn't even started - it is just still in the queue.

Perhaps something like this would be effective. It does require two threads but a TimerTask thread should consume very little.

public class RunWithTimeout {

    public RunWithTimeout(Runnable r, long timeout) {
        // Prepare the thread.
        final Thread t = new Thread(r);
        // Start the timer.
        new Timer(true).schedule(new TimerTask() {

            @Override
            public void run() {
                if (t.isAlive()) {
                    // Abort the thread.
                    t.interrupt();
                }
            }
        }, timeout * 1000);
        // Start the thread.
        t.start();
    }

}

class WaitAFewSeconds implements Runnable {

    final long seconds;

    WaitAFewSeconds(long seconds) {
        this.seconds = seconds;
    }

    @Override
    public void run() {
        try {
            Thread.sleep(seconds * 1000);
        } catch (InterruptedException ie) {
            System.out.println("WaitAFewSeconds(" + seconds + ") - Interrupted!");
        }
    }

}

public void test() {
    new RunWithTimeout(new WaitAFewSeconds(5), 3);
    new RunWithTimeout(new WaitAFewSeconds(3), 5);
}

Here's an alternative that only uses one extra thread.

public class ThreadKiller implements Runnable {

    DelayQueue<WaitForDeath> kill = new DelayQueue<>();

    private class WaitForDeath implements Delayed {

        final Thread t;
        final long finish;

        public WaitForDeath(Thread t, long wait) {
            this.t = t;
            this.finish = System.currentTimeMillis() + wait;
        }

        @Override
        public long getDelay(TimeUnit unit) {
            return unit.convert(finish - System.currentTimeMillis(), TimeUnit.MILLISECONDS);
        }

        @Override
        public int compareTo(Delayed o) {
            long itsFinish = ((WaitForDeath) o).finish;
            return finish < itsFinish ? -1 : finish == itsFinish ? 0 : 1;
        }

    }

    @Override
    public void run() {
        while (true) {
            try {
                WaitForDeath t = kill.take();
                if (t.t.isAlive()) {
                    // Interrupt it.
                    t.t.interrupt();
                }
            } catch (InterruptedException ex) {
                // Not sure what to do here.
            }
        }
    }

    public void registerThread(Thread t, long wait) {
        // Post it into the delay queue.
        kill.add(new WaitForDeath(t, wait));
    }
}

public void test() throws InterruptedException {
    // Testing the ThreadKiller.
    ThreadKiller killer = new ThreadKiller();
    Thread killerThread = new Thread(killer);
    killerThread.setDaemon(true);
    Thread twoSeconds = new Thread(new WaitAFewSeconds(2));
    Thread fourSeconds = new Thread(new WaitAFewSeconds(4));
    killer.registerThread(twoSeconds, 5000);
    killer.registerThread(fourSeconds, 3000);
    killerThread.start();
    twoSeconds.start();
    fourSeconds.start();
    System.out.println("Waiting");
    Thread.sleep(10 * 1000);
    System.out.println("Finished");
    killerThread.interrupt();
}
OldCurmudgeon
  • 64,482
  • 16
  • 119
  • 213
0

You need to start timer when the thread runs. Then no thread in waiting state will be killed. Here is the sample from this thread:

import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class PoolTest {
    class TimeOutTask extends TimerTask {
        Thread t;

        TimeOutTask(Thread t) {
            this.t = t;
        }

        public void run() {
            if (t != null && t.isAlive()) {
                t.interrupt();
            }
        }
    }

    class MyRunnable implements Runnable {
        Timer timer = new Timer(true);

        public void run() {
            timer.schedule(new TimeOutTask(Thread.currentThread()), 1000);
            try {
                System.out.println("MyRunnable...");
                Thread.sleep(10000);
            } catch (InterruptedException ie) {
                System.out.println("MyRunnable error...");
                ie.printStackTrace();
            }
        }
    }

    public static void main(String args[]) {
        new PoolTest();
    }

    public PoolTest() {
        try {
            ExecutorService pe = Executors.newFixedThreadPool(3);
            pe.execute(new MyRunnable());
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}
Hitesh
  • 121
  • 3
  • 14