Problem
I am working on a project in which I am supposed to make synchronous and asynchronous behavior of my client.
The customer will call our client with a userId
and we will construct a URL from that userId
and make an HTTP call to that URL and we will get a JSON string back after hitting the URL. And after we get the response back as a JSON string, we will send that JSON string back to our customer.
In this case, I need to have synchronous and asynchronous methods. Some customers will call the executeSynchronous
method to get the same feature and some customers will call our executeAsynchronous
method to get the data back.
Interface:
public interface Client {
// for synchronous
public String executeSynchronous(final String userId);
// for asynchronous
public Future<String> executeAsynchronous(final String userId);
}
SmartClient
which implements the Client
interface:
public class SmartClient implements Client {
ExecutorService executor = Executors.newFixedThreadPool(5);
// This is for synchronous call
@Override
public String executeSynchronous(String userId) {
String response = null;
Future<String> handle = getHandle(userId);
try {
response = handle.get(500, TimeUnit.MILLISECONDS);
} catch (TimeoutException e) {
}
return response;
}
// This is for asynchronous call
@Override
public Future<String> executeAsynchronous(String userId) {
return getHandle(userId);
}
private Future<String> getHandle(String userId) {
Future<String> future = null;
Task task = new Task(userId);
future = executor.submit(task);
return future;
}
}
Simple class which will perform the actual task:
class Task implements Callable<String> {
private final String userId;
public Task(String userId) {
this.userId = userId;
}
public String call() throws Exception {
String url = createURL(userId);
// make a HTTP call to the URL
RestTemplate restTemplate = new RestTemplate();
String jsonResponse = restTemplate.getForObject(url, String.class);
return jsonResponse;
}
// create a URL
private String createURL(String userId) {
String generateURL = somecode;
return generateURL;
}
}
- Is this the correct and efficient way of doing this problem?
- How about the exception handling?
- Do I need any other
catch
blocks at any places?
Solution
Late answer to this question…..
There are a few things which should be considered when implementing a solution like this, and best-practice comes in to play here.
One nit-pick … before we go further, your interface the methods
executeAsynchronous
andexecuteSynchronous
, whereas your
implementation hasexecuteAsync
andexecute
. This is a carelessly
posted question?
What you have, at the moment, works, but is it the best way? I don’t think so.
You code has a few features that are important:
executeSynchronous()
– waits until you have a result, returns the result.executeAsynchronous()
– returns a Future immediately which can be processed after other things are done, if needed.- it has a private
getHandle()
method which does the hard work of creating a Callable, and returning a future.
Now, I have tried to list why there are problems with this, but, it’s easier to actually show you a different approach, and explain why it is better…. so, here’s a different approach, which reverses the logic.
First thing, we are going to make the interface a generic interface… why does it always have to return a String value for each URL? Also, why is it a Client
? It is not really a client, but it is a RemoteCall
. This is not the full Client, but a part of what it does when it talks to the server…. so, let’s rename the Client
interface, and make it generic…. Also, while we are at it, you need to declare that you throw some form of exception… my preference is for a new checked exception type, but maybe you can borrow an existing exception. What you have is not best-practice. Here’s a decent exception to throw:
class RemoteCallException extends Exception {
private static final long serialVersionUID = 1L;
public RemoteCallException(String message, Throwable cause) {
super(message, cause);
}
public RemoteCallException(String message) {
super(message);
}
}
Then, with that exception, we can complete the interface:
public interface RemoteCall<T> {
// for synchronous
public T executeSynchronous(final String action) throws RemoteCallException;
// for asynchronous
public Future<T> executeAsynchronous(final String action);
}
Right, this interface is now a better representation of the work it is supposed to do.
Now, here’s a key feature I want you to consider…. at the moment, you may only have one ‘action’ you need to execute on the server, the part of translating a userid in to a JSOM response. But, I expect there is, or will be, more. Having to build a new implementation for your Client
interface for every type of operation is a real PITA, especially when it requires building a new Callable
/ Task
instance…. so, what we do is we create a convenient abstract class, which will do all the synchronous/asynchronous work for us….
Note, it does not implement executeSynchronous(), and it is still fully generic. It uses an executor service that is passed in.
One of the really huge advantages of reversing the logic (there is no Callable involved in the synchronous call) is that the work is done on the calling thread. In your implementation, you have two threads fully occupied, the calling thread waits for work to be done, and a thread in the ExecutorService is actually doing the work. By making the executeSynchronous()
call the way I suggest, only one thread is occupied, and the thread that is busy is the same thread that calls executeSynchronous()
public abstract class AbstractRemoteCall<T> implements RemoteCall<T> {
private final ExecutorService executor;
public AbstractRemoteCall(ExecutorService executor) {
this.executor = executor;
}
// note, final so it cannot be overridden in a sub class.
// note, action is final so it can be passed to the callable.
public final Future<T> executeAsynchronous(final String action) {
Callable<T> task = new Callable<T>() {
@Override
public T call() throws RemoteCallException {
return executeSynchronous(action);
}
};
return executor.submit(task);
}
}
OK, so, the above class deals with any asynchronous method calls, and delegates the synchronous calls to the actual implementations.
Now we get to your actual implementation you want to do, the userid
-> JSON
transformation. It is now as easy as:
public class SmartClient extends AbstractRemoteCall<String> {
public SmartClient() {
super(Executors.newFixedThreadPool(5));
}
@Override
public String executeSynchronous(String userId) throws RemoteCallException {
String url = createURL(userId);
// make a HTTP call to the URL
RestTemplate restTemplate = new RestTemplate();
return restTemplate.getForObject(url, String.class);
}
// create a URL
private String createURL(String userId) {
String generateURL = somecode;
return generateURL;
}
}
The advantages of all these changes are:
- the abstract class is the only place you need to worry about threading
- in a synchronous call, the work is done on the calling thread
- there is a better mechanism for error handling with the new RemoteCallException.
- you can now have many different implementations of the abstract class, each implementation does their own type of restful work, and you don’t need to worry about the threading in any of them.
- the actual working classes (like
SmartClient
) do one thing, and one thing only.
There are other advantages…. but, what you have now, though it works, is not nearly as maintainable, extensible, and reliable as what it could be.
EDIT: Some use-cases
How would the code I recommend be used? Two situations…. Synchronous
// set up some system, need the JSON data
SmartClient sc = new SmartClient();
String json = sc.executeSynchronous(userid);
Collecting data asynchronous:
// Set up a number of things, timing can be in parallel.
try {
SmartClient sc = new SmartClient();
Future<String> jsonfut = sc.executeAsynchronous(userid);
// set up some other things.
// perhaps some thing that gets other data....
// this is a different class just like the SmartClient that can be asynchronous
DataCLient dc = new DataClient();
Future<String> defautsfut = dc.executeAsynchronous("getdefaults");
Future<String> messagesfut = dc.executeAsyncrhonous("messages");
String json = jsonfut.get(500, TimeUnit.MILLISECONDS);
String defaults = defaultsfut.get(); // no timeout
String messages = messagesfut.get(); // no timeout
} catch (TimeoutException te) {
// handle appropriately
}
This looks good. A synchronous call is always an async call plus a wait for the result. To make that clear, execute()
should probably call executeAsync()
instead of getHandle()
.