Is this a good practice to init a new httpClient upon each new request

Question

I'm using apache httpClient lib in my java project.

I got an error when I didn't clean the HttpClient

Timeout waiting for connection

public class RoutingUrlHttpClient implements IRoutingUrlHttpClient {

    final static Logger logger = Logger.getLogger(RoutingUrlHttpClient.class);


    private IRoutingResponseFromStringFetcher routingResponseParser;
    private IRoutingResponseConverter routingResponseConverter;
    private IUrlUtils urlUtils;
    private CloseableHttpClient client;
    private ILogUtils logUtils;


    @Inject
    @Singleton
    public RoutingUrlHttpClient(IRoutingResponseFromStringFetcher routingResponseParser,
                                IRoutingResponseConverter routingResponseConverter, IUrlUtils urlUtils,
                                ILogUtils logUtils) {
        this.routingResponseParser = routingResponseParser;
        this.routingResponseConverter = routingResponseConverter;
        this.urlUtils = urlUtils;
        this.logUtils = logUtils;

        RequestConfig requestConfig = RequestConfig.custom()
                //time till handshake
                .setConnectTimeout(40 * 1000)
                //happens when you have a pool of connections and they are all busy, not allowing the connection
                // manager to give you one connection to make the request.
                .setConnectionRequestTimeout(40 * 1000)
                //time till response
                .setSocketTimeout(40 * 1000)
                .build();
        client = HttpClientBuilder
                .create()
                .setDefaultRequestConfig(requestConfig)
                .build();
    }


//    public CompleteRoutingResponseDtoWrapper sendAndReturnDtoWrapper(String routingRequestUrl) {
//        CompleteRoutingResponseDtoWrapper completeRoutingResponseDtoWrapper = sendRoutingRequestString
// (routingRequestUrl);
//        completeRoutingResponseDtoWrapper.requestUrl = routingRequestUrl;
//        return completeRoutingResponseDtoWrapper;
//    }

    @Override
    public CompleteRoutingResponseLong sendRoutingRequestStringWithFullResponse(String routingRequestUrl) {
        CompleteRoutingResponseDtoWrapper completeRoutingResponseDtoWrapper =
                sendRoutingRequestString(routingRequestUrl);
        completeRoutingResponseDtoWrapper.requestUrl = routingRequestUrl;
        return routingResponseConverter.toCompleteRoutingResponseFull(completeRoutingResponseDtoWrapper);
    }



    private CompleteRoutingResponseDtoWrapper sendRoutingRequestString(String routingRequestUrl) {
        return sendRoutingRequestString(Constants.NUM_OF_RETRIES, routingRequestUrl);
    }

    private CompleteRoutingResponseDtoWrapper sendRoutingRequestString(int numberOfTriesLeft,
                                                                       String routingRequestUrl) {
        routingRequestUrl = urlUtils.getHttpUrl(routingRequestUrl);

        CompleteRoutingResponseDtoWrapper answer = new CompleteRoutingResponseDtoWrapper();
        CloseableHttpResponse response = null;
        try {

            logger.debug("before sending http");

            Stopwatch stopWatch = Stopwatch.createStarted();
            response = client.execute(new HttpGet(routingRequestUrl));
            stopWatch.stop();
//            String latencyMsg = "after sending http. client-latency: "+stopWatch.elapsed(TimeUnit.MILLISECONDS) +" server-latency: "+response.getHeaders("Latency")[0].getValue();

            logUtils.addLongToLongStatisticCollector("http.client.latency", (int)stopWatch.elapsed(TimeUnit.MILLISECONDS));
            logUtils.addLongToLongStatisticCollector("http.server.latency", Integer.parseInt(response.getHeaders("Latency")[0].getValue()));

            answer = analyzeStatusCodeAndMsgBody(numberOfTriesLeft, routingRequestUrl, answer, response, stopWatch);


        } catch (Exception e) {
//            e.printStackTrace();
//            System.out.println(e.getMessage());
            answer.errorMsg = e.getMessage();
            answer.latency = null;
        }

        handleNullResponse(answer);
        return answer;
    }

so I changed the code to this

    @Inject
    @Singleton
    public RoutingUrlHttpClient(IRoutingResponseFromStringFetcher routingResponseParser,
                                IRoutingResponseConverter routingResponseConverter, IUrlUtils urlUtils,
                                ILogUtils logUtils) {
        this.routingResponseParser = routingResponseParser;
        this.routingResponseConverter = routingResponseConverter;
        this.urlUtils = urlUtils;
        this.logUtils = logUtils;

        requestConfig = RequestConfig.custom()
                //time till handshake
                .setConnectTimeout(40 * 1000)
                //happens when you have a pool of connections and they are all busy, not allowing the connection
                // manager to give you one connection to make the request.
                .setConnectionRequestTimeout(40 * 1000)
                //time till response
                .setSocketTimeout(40 * 1000)
                .build();
    }


//    public CompleteRoutingResponseDtoWrapper sendAndReturnDtoWrapper(String routingRequestUrl) {
//        CompleteRoutingResponseDtoWrapper completeRoutingResponseDtoWrapper = sendRoutingRequestString
// (routingRequestUrl);
//        completeRoutingResponseDtoWrapper.requestUrl = routingRequestUrl;
//        return completeRoutingResponseDtoWrapper;
//    }

    @Override
    public CompleteRoutingResponseLong sendRoutingRequestStringWithFullResponse(String routingRequestUrl) {
        CompleteRoutingResponseDtoWrapper completeRoutingResponseDtoWrapper =
                sendRoutingRequestString(routingRequestUrl);
        completeRoutingResponseDtoWrapper.requestUrl = routingRequestUrl;
        return routingResponseConverter.toCompleteRoutingResponseFull(completeRoutingResponseDtoWrapper);
    }



    private CompleteRoutingResponseDtoWrapper sendRoutingRequestString(String routingRequestUrl) {
        return sendRoutingRequestString(Constants.NUM_OF_RETRIES, routingRequestUrl);
    }

    private CompleteRoutingResponseDtoWrapper sendRoutingRequestString(int numberOfTriesLeft,
                                                                       String routingRequestUrl) {
        routingRequestUrl = urlUtils.getHttpUrl(routingRequestUrl);

        CompleteRoutingResponseDtoWrapper answer = new CompleteRoutingResponseDtoWrapper();
        CloseableHttpResponse response = null;
        try {

            logger.debug("before sending http");

            Stopwatch stopWatch = Stopwatch.createStarted();


            //try-with-resources
            try (CloseableHttpClient client = HttpClientBuilder
                    .create()
                    .setDefaultRequestConfig(requestConfig)
                    .build()){
                response = client.execute(new HttpGet(routingRequestUrl));
            stopWatch.stop();
//            String latencyMsg = "after sending http. client-latency: "+stopWatch.elapsed(TimeUnit.MILLISECONDS) +" server-latency: "+response.getHeaders("Latency")[0].getValue();

            logUtils.addLongToLongStatisticCollector("http.client.latency", (int)stopWatch.elapsed(TimeUnit.MILLISECONDS));
            logUtils.addLongToLongStatisticCollector("http.server.latency", Integer.parseInt(response.getHeaders("Latency")[0].getValue()));

            answer = analyzeStatusCodeAndMsgBody(numberOfTriesLeft, routingRequestUrl, answer, response, stopWatch);
            }

        } catch (Exception e) {
//            e.printStackTrace();
//            System.out.println(e.getMessage());
            answer.errorMsg = e.getMessage();
            answer.latency = null;
        }

        handleNullResponse(answer);
        return answer;
    }

I wanted to ask:

• Is this a good practice to init a new httpClient upon each new request?

• Am i cleaning the resource correctly?

• Is there more efficient way to send HTTP requests? maybe an nio http client lib?

Answer 1

My request for clarity on this in HTTP Client requests done right suggests that since CloseableHttpClient is thread safe it probably can/should be used as a singleton so - no, you do not need to init a new httpClient.

Would recommend a read of the responses to that question - very useful.

Answer 2

CloseableHttpClient is thread-safe so you can safely reuse instances rather than creating them on a per-request basis. You could also investigate connection-pooling. I quite often use a pattern like this:

PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager();
CloseableHttpClient client = HttpClients.custom().setConnectionManager(connectionManager).build();

Have a read of this for more context.