Synchronous and asynchronous client methods

Question

I am working on a project in which I need to have synchronous and asynchronous methods of my Java client. Some customer will call synchronously and some customer will call asynchronously, depending on their requirement.

Here is my Java client which has synchronous and asynchronous methods:

public class TestingClient implements IClient {

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

    // for synchronous
    @Override
    public String executeSync(ClientKey keys) {

        String response = null;
        try {

            Future<String> handle = executeAsync(keys);
            response = handle.get(keys.getTimeout(), TimeUnit.MILLISECONDS);
        } catch (TimeoutException e) {

        } catch (Exception e) {

        }

        return response;
    }

    // for asynchronous
    @Override
    public Future<String> executeAsync(ClientKey keys) {

        Future<String> future = null;

        try {
            ClientTask ClientTask = new ClientTask(keys, restTemplate);
            future = service.submit(ClientTask);
        } catch (Exception ex) {

        }

        return future;
    }
}

Here is my ClientTask class, which implements the Callable interface. I am passing around the dependency using DI pattern in the ClientTask class. In the call method, I am just making a URL basis on machineIPAddress and using the ClientKeys which is passed to ClientTask class and then hit the server using RestTemplate and get the response back.

class ClientTask implements Callable<String> {

    private ClientKey cKeys;
    private RestTemplate restTemplate;

    public ClientTask(ClientKey cKeys, RestTemplate restTemplate) {
        this.restTemplate = restTemplate;
        this.cKeys = cKeys;
    }

    @Override
    public String call() throws Exception {

        // .. some code here
        String url = generateURL("machineIPAddress");           
        String response = restTemplate.getForObject(url, String.class);

        return response;
    }

    // is this method thread safe and the way I am using `cKeys` variable here is also thread safe?
    private String generateURL(final String hostIPAdress) throws Exception {
        StringBuffer url = new StringBuffer();
        url.append("http://" + hostIPAdress + ":8087/user?user_id=" + cKeys.getUserId() + "&client_id="
            + cKeys.getClientId());

        final Map<String, String> paramMap = cKeys.getParameterMap();
        Set<Entry<String, String>> params = paramMap.entrySet();
        for (Entry<String, String> e : params) {
            url.append("&" + e.getKey());
            url.append("=" + e.getValue());
        }

        return url.toString();
    }
}

Here is my ClientKey class using the builder patter, which the customer will use to make the input parameters to pass to the TestingClient:

public final class ClientKey {

    private final long userId;
    private final int clientId;
    private final long timeout;
    private final boolean testFlag;
    private final Map<String, String> parameterMap;

    private ClientKey(Builder builder) {
    this.userId = builder.userId;
    this.clientId = builder.clientId;
    this.remoteFlag = builder.remoteFlag;
    this.testFlag = builder.testFlag;
    this.parameterMap = builder.parameterMap;
    this.timeout = builder.timeout;
    }

    public static class Builder {
    protected final long userId;
    protected final int clientId;
    protected long timeout = 200L;
    protected boolean remoteFlag = false;
    protected boolean testFlag = true;
    protected Map<String, String> parameterMap;

    public Builder(long userId, int clientId) {
        this.userId = userId;
        this.clientId = clientId;
    }

    public Builder parameterMap(Map<String, String> parameterMap) {
        this.parameterMap = parameterMap;
        return this;
    }

    public Builder remoteFlag(boolean remoteFlag) {
        this.remoteFlag = remoteFlag;
        return this;
    }

    public Builder testFlag(boolean testFlag) {
        this.testFlag = testFlag;
        return this;
    }

    public Builder addTimeout(long timeout) {
        this.timeout = timeout;
        return this;
    }

    public ClientKey build() {
        return new ClientKey(this);
    }
    }

    public long getUserId() {
    return userId;
    }

    public int getClientId() {
    return clientId;
    }

    public long getTimeout() {
    return timeout;
    }

    public Map<String, String> getParameterMap() {
    return parameterMap;
    }

    public boolean istestFlag() {
    return testFlag;
    }
}

Is this thread-safe, as I am using ClientKey variables in ClientTask class in multithreaded environment so not sure what will happen if another thread tries to make ClientKey variable while making a call to TestingClient synchronous method? The customer will be making a call to us with the use of this code, and they can call us from their multithreaded application as well.

IClient testClient = ClientFactory.getInstance();

Map<String, String> testMap = new LinkedHashMap<String, String>();
testMap.put("hello", "world");

ClientKey keys = new ClientKey.Builder(12345L, 200).addTimeout(2000L).parameterMap(testMap).build();

String response = testClient.executeSync(keys);

I'm just trying to understand whether my above code will be thread-safe or not, as they can pass multiple values to my TestingClient class from multiple threads. I have a feeling that my ClientKey class is not thread-safe because of parameterMap, but I'm not sure.

Also, do I need StringBuffer here, or will StringBuilder be fine as StringBuilder is faster than StringBuffer, and because it's not synchronized?

Answer 1

If I haven't missed anything then ClientKey should be thread safe. Once you have build the ClientKey object, the parameterMap is only ever read and never modified.

However it relies on the fact that no malicious or buggy client isn't trying to modify in the meantime. Therefor I would consider changing the ClientKey object to protect against changes. Several options come to mind

1. Change your ClientKey constructor so it creates a readonly map:

   private ClientKey(Builder builder) {
       this.userId = builder.userId;
       this.clientId = builder.clientId;
       this.remoteFlag = builder.remoteFlag;
       this.testFlag = builder.testFlag;
       this.parameterMap = Collections.unmodifiableMap(builder.parameterMap);
       this.timeout = builder.timeout;
   } 
   

   This will prevent anyone from changing the map obtained via getParameterMap.

2. Make your ClientKey class implement Iterable<Entry<string, string>> which allows iteration over the internal map rather than exposing the map through a get function.

3. Return an Iterable<Entry<string, string>> from getParameterMap (and probably call it getParameters instead).

I'd prefer 2 or 3 because it enforces the readonly-ness in a much stronger way.

Update

Number 3 could be simply implemented as

Iterable<Entry<string, string>> getParameters() {
    return parameterMap.entrySet();
}

or if you want to be really paranoid (to protect against someone trying to cast the return value back to a Set in order to try adding elements):

Iterable<Entry<string, string>> getParameters() {
    return Collections.unmodifiableSet(parameterMap.entrySet());
}

Usage would be to replace this:

   final Map<String, String> paramMap = cKeys.getParameterMap();
    Set<Entry<String, String>> params = paramMap.entrySet();
    for (Entry<String, String> e : params) {
        url.append("&" + e.getKey());
        url.append("=" + e.getValue());
    }

with:

    for (Entry<String, String> e : cKeys.getParameters()) {
        url.append("&" + e.getKey());
        url.append("=" + e.getValue());
    }

The slight downside to that is that a user cannot perform fast lookups anymore on it to get the value for a specific parameter. If you want to support that then you'd need to add a getParameter(string parameterName) method to ClientKey.