Efficiently having a sync and async method by implementing future class

Question

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<DataResponse> 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<DataResponse> 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, dataKey);
            dataResponse = new DataResponse(null, DataErrorEnum.CLIENT_ERROR, DataStatusEnum.ERROR);
        }

        return dataResponse;
    }

    //for asynchronous call
    @Override
    public Future<DataResponse> executeAsynchronous(DataKey dataKey) {
        DataClientFuture dataFuture = null;

        try {
            Task task = new Task(dataKey, restTemplate);
            Future<DataResponse> future = executor.submit(task);
            dataFuture = new DataClientFuture(future, dataKey);             
        } catch (Exception ex) {
            PotoLogging.logErrors(ex, DataErrorEnum.CLIENT_ERROR, dataKey);
        }

        return dataFuture;
    }
}

DataClientFuture class:

(Does this look right or can I add more stuff to this?)

public class DataClientFuture implements Future<DataResponse> {
    private final Future<DataResponse> delegate;
    private final DataKey dataKey;

    public DataClientFuture(Future<DataResponse> response, DataKey dataKey) {
        this.delegate = response;
        this.dataKey = dataKey;
    }

    @Override
    public boolean cancel(boolean mayInterruptIfRunning) {
        return delegate.cancel(mayInterruptIfRunning);
    }

    @Override
    public boolean isCancelled() {
        return delegate.isCancelled();
    }

    @Override
    public boolean isDone() {
        return delegate.isDone();
    }

    // should I still throw exception here and catch it as well?
    @Override
    public DataResponse get() throws InterruptedException, ExecutionException {
        DataResponse dataResponse = null;
        try {
            dataResponse = delegate.get();
        } catch (Exception ex) {
            PotoLogging.logErrors(ex, DataErrorEnum.CLIENT_ERROR, dataKey);
            dataResponse = new DataResponse(null, DataErrorEnum.CLIENT_ERROR, DataStatusEnum.ERROR);
        }
        return dataResponse;
    }

    // should I still throw exception here and catch it as well?
    @Override
    public DataResponse get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException,
            TimeoutException {
        DataResponse dataResponse = null;
        try {
            dataResponse = delegate.get(timeout, unit);
        } 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;
    }
}

Simple class which will perform the actual task:

public class Task implements Callable<DataResponse> {

    private DataKey dataKey;
    private RestTemplate restTemplate;

    public Task(DataKey dataKey, RestTemplate restTemplate) {
        this.dataKey = dataKey;
        this.restTemplate = restTemplate;
    }

    @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;
    }

    // create a URL by using dataKey object
    private String createURL() {
        String url = somecode;

        return url;
    }
}

As I mentioned above, some customers will call executeSynchronous method to get the data for that user id which they are passing in DataKey object and some customers will call executeAsynchronous method with DataKey object but in latter case, they will do future.get in their code base.

If you see my executeSynchronous method, I am doing future.get after calling executeAsynchronous method and if there is any TimeoutException, then I am logging using PotoLogging class which is specific in our company and that logs will go to some storage system in our company which we use to look all our error logs on the dashboard from our library. And it mainly depends how we are logging it with what names so that we can see those names in the dashboard.

Now the problem is customer within our company can also call executeAsynchronous method but that means, they will do future.get in their code base and that can also result in TimeoutException and I wanted to log this TimeoutException as well from our library in the way I wanted so that it can show up in our storage system on the dashboard and because of that reason I created DataClientFuture class and implemented most of the method in that instead of using default future.get implementation.

This logging will log into our storage system.

PotoLogging.logErrors(ex, DataErrorEnum.TIMEOUT_ON_CLIENT, dataKey);

I will be running this code in production.

Answer 1

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.