Efficiently having a sync and async method by implementing future class

I need to make a library in which I will have synchronous and asynchronous methods in it. And then if there are any exception like TimeoutException or any other exception, I need to log an error into our company's storage system with the logging I have both from synchronous and asynchronous method.

Core Logic of my Library

The customer will use our library and they will call it by passing DataKey builder object. We will then construct a URL by using that DataKey object and make a HTTP client call to that URL by executing it and after we get the response back as a JSON String, we will send that JSON String back to our customer as it is by creating DataResponse object.

Some customer will call the executeSynchronous method to get the same feature and some customer will call our executeAsynchronous method and with the latter they will do future.get in their code base.

Interface:

public interface Client {

    // for synchronous
    public DataResponse executeSynchronous(DataKey dataKey);

    // for asynchronous
    public Future executeAsynchronous(DataKey dataKey);
}

And then I have my DataClient which implements the above Client interface:

```
public class DataClient implements Client {

private RestTemplate restTemplate = new RestTemplate();
private ExecutorService service = Executors.newFixedThreadPool(10);

// for synchronous call
@Override
public DataResponse executeSynchronous(DataKey dataKey) {
DataResponse dataResponse = null;

try {
Future future = executeAsynchronous(dataKey);
dataResponse = future.get(dataKey.getTimeout(), TimeUnit.MILLISECONDS);
} catch (TimeoutException ex) {
PotoLogging.logErrors(ex, DataErrorEnum.TIMEOUT_ON_CLIENT, dataKey);
dataResponse = new DataResponse(null, DataErrorEnum.TIMEOUT_ON_CLIENT, DataStatusEnum.ERROR);
} catch (Exception ex) {
PotoLogging.logErrors(ex, DataErrorEnum.CLIENT_ERROR,

You should rethrow any InterruptedException, they are meant to shutdown the current thread or in the case of Task.call signal cancelation of the task.

I would change execute synchronous to just creating the Task and calling it without messing with the executor:

@Override
public DataResponse executeSynchronous(DataKey dataKey) {
    DataResponse dataResponse = null;

    try {
        Task task = new Task(dataKey, restTemplate);

        dataResponse = task.call();//direct call
    } catch (TimeoutException ex) {
        PotoLogging.logErrors(ex, DataErrorEnum.TIMEOUT_ON_CLIENT, dataKey);
        dataResponse = new DataResponse(null, DataErrorEnum.TIMEOUT_ON_CLIENT, DataStatusEnum.ERROR);
    } catch (Exception ex) {
        PotoLogging.logErrors(ex, DataErrorEnum.CLIENT_ERROR, dataKey);
        dataResponse = new DataResponse(null, DataErrorEnum.CLIENT_ERROR, DataStatusEnum.ERROR);
    }

    return dataResponse;
}

Your task will swallow any exception that may be thrown, the only further effect is that result will be set to a response. Instead don't catch only the RestException in your Task and only catch the ExecutionException after future.get to set the result to failed in case of another error.

@Override
public DataResponse get() throws InterruptedException {
    DataResponse dataResponse = null;
    try {
        dataResponse = delegate.get();
    } catch (ExecutionException ex) {
        DataErrorEnum error = DataErrorEnum.CLIENT_ERROR;

        PotoLogging.logErrors(ex.getCause(), error, dataKey);
        dataResponse = new DataResponse(null, error, DataStatusEnum.ERROR);
    }
    return dataResponse;
}

similarly for timed get, don't catch interrupted or timeout, they are artifacts of the calling code and let them deal with it.

StackExchange Code Review Q#83970, answer score: 6