11
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Didn't you post this earlier? \$\endgroup\$ – Dan Pantry Mar 13 '15 at 20:40
  • \$\begingroup\$ @DanPantry Yes I did but that question was incomplete and I missed few important stuff in that so I rephrased my question and posted it here by adding more details but I deleted the old one so that there is no duplicate. \$\endgroup\$ – david Mar 13 '15 at 22:17
  • \$\begingroup\$ @Jamal sure I will make sure on that. \$\endgroup\$ – david Mar 13 '15 at 22:17
  • \$\begingroup\$ Your design is becoming hugely complicated by allowing asynchronous access. Why not provide just a synchronous interface, and have your clients multithread should they need to? \$\endgroup\$ – kiwiron Mar 14 '15 at 6:38
  • \$\begingroup\$ @kiwiron I don't agree to your point. If you look at some of open source project (like datastax java driver), they also have sync and async method both and depending on what is suited to the application, we use that. \$\endgroup\$ – david Mar 14 '15 at 6:54
5
\$\begingroup\$

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 :)

\$\endgroup\$
  • \$\begingroup\$ Thanks Mat. I already have my JavaDoc on those method, I didn't pasted here because then it will become too long in the question so to make it readable and understandable, I made it simple. And regarding your methods name, I will definitely change that to what you have suggested. \$\endgroup\$ – david Mar 13 '15 at 22:30
  • \$\begingroup\$ Don't worry about long posts! As you know anything in the question's code can be subject to review, including comments. Next time just copy the code straight from your IDE - if JavaDoc is making your code hard to read/understand, that's probably worthy of something to say in an answer, too! Cheers :) \$\endgroup\$ – Mathieu Guindon Mar 13 '15 at 22:37
  • \$\begingroup\$ Very well understood. I will make sure on this next time. \$\endgroup\$ – david Mar 13 '15 at 22:40
5
\$\begingroup\$

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:

\$\endgroup\$
  • \$\begingroup\$ First of all thanks a lot for spending time on my question and providing very detailed review. I learned two-three different things from your 10 points. Let's talk about those points. Once you are around, I will talk to you in detail whenever you are free on this. Point 1 - Yes I agree to your point. In general in our company some customer will call async and they will do something in the meantime and then get the data from future. Point 2 - That's a really great point which I was totally not aware of. This is what I learned something new and I have added a fix for that like this - \$\endgroup\$ – david Mar 16 '15 at 5:26
  • \$\begingroup\$ future.cancel(true). Third Point - I am not sure on this point, I need tot get more info from you on this why it has to be daemon thread and will there be any impact if I continue to use what I am using now. Point 4 - Yeah that's what I was also thinking and that's why I wanted to get this code reviewed. The only reason to implement future is so that I can log TimeoutException in our company storage system if anyone is calling async method and then doing future.get in their code base. If I don't implement future interface, then it will use default jdk implementation and I won't be log \$\endgroup\$ – david Mar 16 '15 at 5:31
  • \$\begingroup\$ anything if tasks gets timedout. Point Five - Not sure on this, need to check with you. Point Six - Yup that's my mistake. Sorry about that. Point Seven - Confuse on this but I guess it's related to throwing Exception in my Task class? Point Eight - Sure understood. Point Nine - This is what I also need to understand better once you are around and how can I overcome this. Point Ten - How do I deal with my current situation? Whenever you are around, we can chat on this. This will definitely help me to know few more stuff and I can improve myself much more. Appreciated all your help. \$\endgroup\$ – david Mar 16 '15 at 5:35
  • \$\begingroup\$ are you around for quick chat? \$\endgroup\$ – david Mar 17 '15 at 21:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.