Using RestTemplate with HttpComponentsClientHttpRequestFactory in a multithreaded environment

Question

I am working on a project in which I need to make an HTTP URL call to my server, running RESTful service, which returns the response as a JSON String. I am using RestTemplate here along with HttpComponentsClientHttpRequestFactory to execute a URL.

I have set up an HTTP request timeout (READ and CONNECTION time out) on my RestTemplate by using HttpComponentsClientHttpRequestFactory.

Here is my main code which is using the future and callables:

public class TimeoutThreadExample {

    private ExecutorService executor = Executors.newFixedThreadPool(10);
    // does it have to be static final?
    private static final RestTemplate restTemplate = createRestTemplate();

    // does this look right with the way I am creating `RestTemplate`?
    private static RestTemplate createRestTemplate(){
        // is it ok to create a new instance of HttpComponentsClientHttpRequestFactory everytime?
       HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
       requestFactory.setReadTimeout(READ_TIME_OUT);
       requestFactory.setConnectTimeout(CONNECTION_TIME_OUT);
       return new RestTemplate(requestFactory);
     }

    public String getData() {
        Future<String> future = executor.submit(new Task(restTemplate));
        String response = null;

        try {
            response = future.get(500, TimeUnit.MILLISECONDS);
        } catch (TimeoutException e) {
            e.printStackTrace();
        } catch (InterruptedException e) {
            e.printStackTrace();
        } catch (ExecutionException e) {
            e.printStackTrace();
        }

        return response;
    }
}

Here is my Task class which implements the Callable interface and uses the RestTemplate:

class Task implements Callable<String> {

    private RestTemplate restTemplate;

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

    public String call() throws Exception {

        String url = "some_url";
        String response = restTemplate.getForObject(url, String.class);

        return response;
    }
}

Now I have everything working in my above code. Is the way I am creating RestTemplate along with HttpComponentsClientHttpRequestFactory thread-safe and efficient? Since RestTemplate is very heavy to be created so not sure whether I have it right or not. I'd like to improve anything in creation of RestTemplate with HttpComponentsClientHttpRequestFactory.

Answer 1

Will you have many instances of TimeoutThreadExample in your real application? If not, then the restTemplate field will be better non-static, initialized in the constructor:

private final RestTemplate restTemplate;

public TimeoutThreadExample() {
    HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
    requestFactory.setReadTimeout(READ_TIME_OUT);
    requestFactory.setConnectTimeout(CONNECTION_TIME_OUT);
    restTemplate = new RestTemplate(requestFactory);
}

On the other hand, if you will have many instances of TimeoutThreadExample, then it will be better to make restTemplate static, and initialize it in a static initializer block instead of using a method:

private static final RestTemplate restTemplate;

static {
    HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
    requestFactory.setReadTimeout(READ_TIME_OUT);
    requestFactory.setConnectTimeout(CONNECTION_TIME_OUT);
    restTemplate = new RestTemplate(requestFactory);
}

As for this question:

// is it ok to create a new instance of HttpComponentsClientHttpRequestFactory everytime?

In both alternatives I offered, the instance is really created only once, so that's ok.

The way I am creating RestTemplate along with HttpComponentsClientHttpRequestFactory is thread safe and efficient?

Initializing static variables is thread safe. And since you're creating RestTemplate and HttpComponentsClientHttpRequestFactory only once, it's efficient too.

Answer 2

What properties should be static?

As a rule of thumb; Do not use static properties. If you ask yourself "Should I use static here?", then you shouldn't. In other words; if it is not a constant, it need not be static either. You asked "does it have to be static final?", in general if a property is static it also needs to be final. However, having a single object shared across multiple objects is not a reason (excuse) to use static properties.

Static properties increases the coupling of component.

What to do?

Use Dependency Injection. Especially if you are using Spring you should use dependency injection. Spring framework libraries are designed to be used that way.

Other points

Follow Dependency Inversion Principle. Concretely: first write the interface you need, then write an implementation.

Do not swallow exceptions, printStackTrace is not proper exception handling.

Do not use local variables unnecessarily, it impedes readability.

Return early from a method (getData()), do not contort your method's body to make it have a single return.

I assume you know what you are doing by creating a Future and immediately calling get.

Refactored Code

Create your component interface, name it according to what it does, not how it does it:

public interface DataGetterDAO {
    String getData();
}

Create your component interface, You can name it according to how it does its work, it that's what sets this implementation apart:

public class DataGetterDAORestImpl implements DataGetterDAO {
    // this could be injected also.
    private ExecutorService executor = Executors.newFixedThreadPool(10);

    private final RestTemplate restTemplate;

    public DataGetterDAORestImpl(RestTemplate restTemplate) {
        this.restTemplate = restTemplate;
    }

    @Override
    public String getData() {
        Future<String> future = executor.submit(new Callable<String>() {
            @Override
            public String call() throws Exception {
                return restTemplate.getForObject("some_url", String.class);
            }
        });

        try {
            return future.get(500, TimeUnit.MILLISECONDS);
        } catch (TimeoutException | InterruptedException | ExecutionException e) {
            throw new RuntimeException("some message", e);
        }
    }
}

Code for using an object does not belong in that object. Because an object should not constrain how itself is used. Object construction and wire-up is such a case of use.

For example above component can be constructed as below, (obtained by moving the rest of the code you supplied):

public class ApplicationWireup {
    void init() {
       HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
       requestFactory.setReadTimeout(READ_TIME_OUT);
       requestFactory.setConnectTimeout(CONNECTION_TIME_OUT);

       RestTemplate restTemplate = new RestTemplate(requestFactory);

       DataGetterDAO dataGetterDAO = new DataGetterDAORestImpl(restTemplate);

       // inject dataGetterDAO to services that need it.
    }
}

or using spring configuration xmls, or annotations, or some other DI framework.

Answer 3

Why not configuring a proper restTemplate with everything you need?

See here - it uses PoolingHttpClientConnectionManager where you can configure the number of connections (and remove your thread executor), also read/connect timeouts and many useful stuff.

This way you will make the code much cleaner.