Your concept is flawed in a few different ways.
One of the ways it is flawed is because you assume that someone calling the async method will actually call "get()" on the Future. This is not certain. Many times, with async systems, it is convenient to submit&forget a task especially if you don't care what the results are....
A second way, is that you don't terminate Tasks that have timed out. You report the timeout to the client, but the task continues to run in the thread pool (potentially occupying one of your limited 10 threads for a long time).
A third problem is that your threads are all non-daemon threads, and you don't provide a clean way to shut down your executor service. Someone will have to call System.exit() in order to terminate your application.
A fourth problem is that extending the Future is just a "hacky" solution. There has to be a better way.....
A fifth problem is that you do not honour the timeout in an asynchronous call. The timeout is part of the DataKey, yet, you don't time out unless a synchronous call is used.
A sixth problem is your poor in-place edits and typos. You create an ExecutorService called service, but then, in your async call, you use: executor.submit(task) ... what's with that?
A seventh problem is that a client-fail in a synchronous call, is logged twice, once in the task, again in the async call.
An eighth problem is that your task call() method throws Exception when only RuntimeException is possible.
A ninth problem is that your timeout starts from async call time, and not from when the task actually starts. If you have 10 threads that are all 'busy' when you call, then you have to wait for space to come available on thee executor service, and that wait time is counted toward the timeout. You should really only have the timeout start when the server is called.
A tenth problem is that you treat InterruptedException as just an Exception. It is not, and it needs special handling.
##Threads
Non-daemon threads are a real problem when it comes to handling an orderly shutdown of an application. You should get in to the habit of creating daemon threads, and knowing when they should be used (not every thread should be a daemon thread). In your case, though, you should only have Daemon threads in your executor service.
In Java8 this is easy...
private static final AtomicInteger threadId = new AtomicInteger();
private static final Thread daemonThread(Runnable torun) {
Thread t = new Thread(torun, "DataClient Thread " + threadId.incrementAndGet());
t.setDaemon(true);
return t;
}
private ExecutorService service = Executors.newFixedThreadPool(10, DataClient::daemonThread);
The above code gives each thread a useful name, and makes them daemon threads.
Even in Java7 it is not hard, just change the call to the newFixedThreadPool:
private ExecutorService service = Executors.newFixedThreadPool(10, new ThreadFactory() {
@Override
public Thread newThread(Runnable r) {
return daemonThread(r);
}
});
Note that the 10 value is a magic number, and it should probably be declared as a constant, or as a config value from some config source:
private static final int THREAD_COUNT = SomeConfig.getIntConfig("dataclient.threadcount");
##Task
Your task thread does too much, and also not enough. You throw exception from a method that catches exception.... here's your code:
// do I need to throw an Exception here, since I am already catching it? @Override public DataResponse call() throws Exception { DataResponse dataResponse = null; String response = null; try { String url = createURL(); response = restTemplate.getForObject(url, String.class); // it is a successful response dataResponse = new DataResponse(response, DataErrorEnum.NONE, DataStatusEnum.SUCCESS); } catch (RestClientException ex) { PotoLogging.logErrors(ex, DataErrorEnum.SERVER_DOWN, dataKey); dataResponse = new DataResponse(null, DataErrorEnum.SERVER_DOWN, DataStatusEnum.ERROR); } catch (Exception ex) { PotoLogging.logErrors(ex, DataErrorEnum.CLIENT_ERROR, dataKey); dataResponse = new DataResponse(null, DataErrorEnum.CLIENT_ERROR, DataStatusEnum.ERROR); } return dataResponse; }
Let's rationalize that:
@Override
public DataResponse call() {
try {
String json = restTemplate.getForObject(createURL(), String.class);
// it is a successful response
return new DataResponse(json, DataErrorEnum.NONE, DataStatusEnum.SUCCESS);
} catch (RestClientException ex) {
PotoLogging.logErrors(ex, DataErrorEnum.SERVER_DOWN, dataKey);
return new DataResponse(null, DataErrorEnum.SERVER_DOWN, DataStatusEnum.ERROR);
} catch (RuntimeException ex) {
PotoLogging.logErrors(ex, DataErrorEnum.CLIENT_ERROR, dataKey);
return new DataResponse(null, DataErrorEnum.CLIENT_ERROR, DataStatusEnum.ERROR);
}
}
Note, there is no exception thrown at all. We catch RuntimeException, and any other exceptions will now cause a compile error.
##Timeouts
Your DataKey class is a container for the timeout for the operation. You specify the timeout in milliseconds. It is now "best practice" to supply both the TimeUnit and the timeout together, so people can set a timeout of, for example 5, TimeUnit.SECONDS. Your DataKey should have a TimeUnit, and should have a corresponding get call too.
Also, problem 9 is about when to start counting toward the timeout. Adding the timeout to the task code solves that problem, as well as the fifth problem too - timeouts on async calls. Also, a good opportunity to work on the tenth - InterruptedException....
I have messed a bit with your code, and have a strategy with some promise, but it is not ready to show, and I need to be gone for the day.....
There's enough here so far for you to work on corrections, and possibly a follow-on question.