HTTP Client requests done right

As advised by many, I am using a client pool - specifically the Apache PoolingHttpClientConnectionManager.

For simplicity I wrap it in my own simple singleton class. Sorry about the rather OTT Stop mechanism:

```
public final class HttpClientPool {
private static final Logger log = LoggerFactory.getLogger(HttpClientPool.class);

// Single-element enum to implement Singleton.
private static enum Singleton {
// Just one of me so constructor will be called once.
Client;
// The thread-safe client.
private final CloseableHttpClient threadSafeClient;
// The pool monitor.
private final IdleConnectionMonitorThread monitor;

// The constructor creates it - thus late
private Singleton() {
PoolingHttpClientConnectionManager cm = new PoolingHttpClientConnectionManager();
// Increase max total connection to 200
cm.setMaxTotal(200);
// Increase default max connection per route to 20
cm.setDefaultMaxPerRoute(20);
// Build the client.
threadSafeClient = HttpClients.custom()
.setConnectionManager(cm)
.build();
// Start up an eviction thread.
monitor = new IdleConnectionMonitorThread(cm);
// Don't stop quitting.
monitor.setDaemon(true);
monitor.start();
}

public CloseableHttpClient get() {
return threadSafeClient;
}

}

public static CloseableHttpClient getClient() {
// The thread safe client is held by the singleton.
return Singleton.Client.get();
}

// Watches for stale connections and evicts them.
private static class IdleConnectionMonitorThread extends Thread {
// The manager to watch.
private final PoolingHttpClientConnectionManager cm;
// Use a BlockingQueue to stop everything.
private final BlockingQueue stopSignal = new ArrayBlockingQueue(1);

// Pushed up the queue.
private static class Stop {
// The return queue.
private final BlockingQueue stop = new ArrayBlock

I'm not an expert in the library or multithreading, so maybe some advises will not be applicable.

TL;DR

I didn't find anything in your code really wrong. In fact, the more I look at it, the more I can just find only nitpicking things, and I'm really forcing myself. I would find myself quite happy to maintain this code. Everything is clean, the exception managing is excellent and it's quite easy to read.

readResponse Looks clean to me. What is important when working with the request is to always "consume" the entity and/or close the request. At first, I thought that you had a problem when the request didn't return a 200. When the return code is not 200 you're not consuming the Entity which can normally block the connection, but it seems that you don't need to consume the Entity if you close() the request. In my code, I'm still using EntityUtils.consume(), but it's probably overkill.

query seems a bit weird to me. I always see either get or post not both. I find weird that query can do both, since normally it's very two different things. Despite that fact, it still really well handled.

This line is a bit hard to fully grasp at first read IMO :

return readResponse(request, HttpClientPool.getClient().execute(request), r);

I find it hard to see that this it's actually where you're making the request. It could be in it's own line, to better extract it's important role. I find it easier to debug too.

final HttpRequestBase request;
//postRequest.addHeader("Accept-Encoding", "gzip,deflate");

I guess the comment is a dead one. It could be remove since it serve no purpose. In general, I find there is a bit too much comments for my own taste, but this is basically because your code is very clean and comments are just expressing what I understand by just reading your code. (I'm really impress by the quality)

// Post it and wait.

This one is not quite true, because you could be executing a Get request and not a Post. I'm starting to think that those methods were at first working with Post request and evolve into both type of request. (base on the dead comment and this one)

if (response.getStatusLine().getStatusCode() == 200)

Instead of 200 I would personaly use HttpStatus.OK.

StackExchange Code Review Q#49151, answer score: 7