Having sync and async methods by implementing future interface

Question

I need to make a library in which I will have synchronous and asynchronous methods in it and this library will be used by our customer in our company. Some customer will call the executeSynchronous method to get the same feature and some customer will call our executeAsynchronous method and with the latter method they will do future.get in their code base.

• executeSynchronous() - waits until I have a result, returns the result.
• executeAsynchronous() - returns a Future immediately which can be processed after other things are done, if needed.

In case of any exception, I need to log it into our company storage system with the logging I have both from synchronous and asynchronous method. This will ensure that I can log the way I want into our storage system, instead of asking each customer to do that.

This logging will log into our storage system.

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

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.

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?
    // Since I need to log an exception into our company's storage system 
    // so I need to log it the way I want to log the exception instead of asking customer to do that
    // so that's why I am catching it.
    // May be after catching I should re-throw the same exception?
    @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?
    // Since I need to log an exception into our company's storage system 
    // so I need to log it the way I want to log the exception instead of asking customer to do that
    // so that's why I am catching it.      
    // May be after catching I should re-throw the same exception?      
    @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;
    }

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

    // 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.

Answer 1

The comments on the interface should be JavaDoc so your IDE picks them up:

public interface Client {

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

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

That said, such comments don't add much value, and the naming of the methods is actually hindering readability IMO. Blame it on my C# background, but I'd rather see this:

public interface Client {    
    public DataResponse execute(DataKey dataKey);
    public Future<DataResponse> executeAsync(DataKey dataKey);
}

That way it's clear that the xxxAsync method is for async execution, and if you make that a convention throughout your code base, then you don't need a Synchronous suffix anywhere.

I'll leave the rest to Java reviewers :)

Answer 2

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.

Resources:

• InterruptedException tutorial
• InterruptedException document on dev works - much better explanation
• Future.get()
• Daemon threads on SO
• Thread Docs