Synchronous and asynchronous methods in a client library

Question

I am working on a library in which I need to make synchronous and asynchronous methods in my client library.

My library does this:

The customer will use our library and they will call it by passing user_id. We will then construct a URL by using that userId and make an HTTP client call to that URL. We will then get a JSON string back after hitting the URL. And after we get the response back as a JSON string, then we will send that JSON String back to our customer as it is.

Now I need to have synchronous and asynchronous methods. Some customer will call the executeSynchronous method to get the same feature and some customer will call our executeAsynchronous method and with the executeAsynchronous method, they will call future.get in there code itself.

Interface:

public interface Client {

    // for synchronous
    public String executeSynchronous(final String userId);

    // for asynchronous
    public Future<String> executeAsynchronous(final String userId);
}

And then I have SmartClient which implements the Client interface:

public class SmartClient implements Client {

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

    // for synchronous call
    @Override
    public String executeSynchronous(String userId) {

        String response = null;

        try {
            Future<String> handle = executeAsynchronous(userId);
            response = handle.get(500, TimeUnit.MILLISECONDS);
        } catch (TimeoutException e) {
            e.printStackTrace();
        }

        return response;
    }

    //for asynchronous call
    @Override
    public Future<String> executeAsynchronous(String userId) {
        Future<String> future = null;

        try {
            Task task = new Task(userId, restTemplate);
            future = executor.submit(task);
        } catch (Exception ex) {
            e.printStackTrace();
        }

        return future;
    }
}

This simple class will perform the actual task:

class Task implements Callable<String> {

    private final String userId;
    private RestTemplate restTemplate;

    public Task(String userId, RestTemplate restTemplate) {
        this.userId = userId;
        this.restTemplate = restTemplate;
    }

    public String call() throws Exception {

        String url = createURL(userId);

        // make a HTTP call to the URL
        String jsonResponse = restTemplate.exchange(url, HttpMethod.GET, null, String.class);

        return jsonResponse;
    }

    // create a URL
    private String createURL(String userId) {
        String generateURL = somecode;

        return generateURL;
    }
}

1. Is this the correct and efficient way of solving this problem? Can we generalize my interface?
2. How about the exception handling?

I need to have both synchronous and asynchronous methods so that the customer can call any method they like to call. Some customers might call the asynchronous method and then they will do future.get in their own library. Some can call the synchronous method, which internally can call the asynchronous method and do a future.get on it to return the response.

Answer 1

public String executeSynchronous(final String userId);

Drop this final as it helps nobody.

2. How about the exception handling?

Exception handling is always a pain, especially with checked exceptions. Usually, the best way is the simplest one, i.e., do nothing. Declare the exception to be thrown and let it blow. Someone up the stack will take care of it (it may be you) in a place where this makes sense.

e.printStackTrace();

This is rather wrong. You should log the message, printing to stderr may be redirected to somewhere where nobody sees it. What's worse, your method returns null and the caller has no way to find out what went wrong. Even worse, the caller does not expect null and will get an NPE one day (usually, when it creates the biggest damage).

@Override
public String executeSynchronous(String userId) {

    String response = null;

    try {
        Future<String> handle = executeAsynchronous(userId);
        response = handle.get(500, TimeUnit.MILLISECONDS);
    } catch (TimeoutException e) {
        e.printStackTrace();
    }

    return response;
}

You're actually using two threads (the current one and one in the executor) where just one is needed. It should go the other way round: Let executeSynchronous do the work and submit a Callable calling it when asynchronous execution is needed.

---

1. Is this the correct and efficient way of doing this problem?

I guess, it's correct, but concerning efficiency it's no good to block two threads instead of one (and there's also the small overhead of executor, which is useless in the synchronous case).

... Can we generalize interface?

Maybe... I guess you want many such methods... and an interface containing both versions would be twice as big; no good idea. There's

com.google.common.util.concurrent.Futures.immediateFuture(@Nullable V value)

in Guava, which could help. Instead of two methods, use two implementations of a single interface (synchronous and asynchronous).

public interface Client {
    public Future<String> executeAsynchronous(String userId);
}

The synchronous implementation does the real job normally, but returns an immediateFuture instead of the value itself.

The asynchronous implementation calls the synchronous one using the executor. In case of 10+ methods I'd consider using a dynamic proxy.

Alternatively, you could add an argument

enum Synchronicity {SYNCHRONOUS, ASYNCHRONOUS}

(don't use a boolean as its meaning is not obvious) to all the interface methods.

Answer 2

The system you have for both the synchronous and asynchronous methods is how I would do it, with the core logic implemented in the asynchronous method; and then have the synchronous method simply do the wait.

The timeout value is hard-coded. This is unusual. I would make the timeout a parameter to the method:

public String executeSynchronous(String userId, long timout, TimeUnit timeUnit)

And then delay as long as is specified. It may make sense to have a default value available as well.

The actual code in these methods, and the error-handling, can be improved a lot. For example, the method:

public Future<String> executeAsynchronous(String userId) {
    Future<String> future = null;

    try {
        Task task = new Task(userId, restTemplate);
        future = executor.submit(task);
    } catch (Exception ex) {
        e.printStackTrace();
    }

    return future;
}

Should be written as:

public Future<String> executeAsynchronous(String userId) {
        return executor.submit(new Task(userId, restTemplate));
}

Note the following:

• neither submit(...) nor new Task(...) throw an exception, so the only possible exception is a runtime exception, and there's no reason to trap those in this situation.
• creating the short-lived task variable is not useful, especially when the code is so short already.
• there's no need for a separate Future variable.
• there's no longer a way to return a null Future.

The synchronous version requires a slightly different approach given the TimeoutException you have.... This is what you have currently:

public String executeSynchronous(String userId) {

    String response = null;

    try {
        Future<String> handle = executeAsynchronous(userId);
        response = handle.get(500, TimeUnit.MILLISECONDS);
    } catch (TimeoutException e) {
        e.printStackTrace();
    }

    return response;
}

I would have:

public String executeSynchronous(String userId) {
    try {
        return executeAsynchronous(userId).get(500, TimeUnit.MILLISECONDS);
    } catch (TimeoutException e) {
        e.printStackTrace();
        return null;
    }
}

Note that the exception handling with just the e.printStackTrace() is weak, and you should have proper logging for that.