Reusing javax.ws.rs.client.Client

Question

I have just ran a profiler on my code and I noticed that I spend a lot of time creating new clients using ClientBuilder.newClient(). This isn't that surprising as the documentation states that it is a heavy operation and that you should reuse the client if possible.

So I created a simple resource pool that should manage all of my Clients

public class SimpleClientResourcePool implements ResourcePool<Client> {
    private LinkedList<Client> pool = new LinkedList<>();

    @Override
    public Optional get() {
        if(pool.isEmpty())
            return Optional.empty();
        return Optional.of(pool.getFirst());
    }

    @Override
    public void release(Client value) {
        pool.addLast(value);
    }
}

and I use it like this

    Client client = clientResourcePool.get().orElseGet(ClientBuilder::newClient);
    Response response = get(countryId, uri, client);

    try {
      ///random code
    } finally {
        response.close();
        clientResourcePool.release(client);
    }

When running locally with just me as a user I am not seeing any problems, but during load there seems to be a concurrency issue that I am not quite understanding.

I am getting an exception that seems to be based in this error:

Caused by: java.lang.IllegalStateException: Connection is still allocated
        at org.apache.http.util.Asserts.check(Asserts.java:34)
        at org.apache.http.impl.conn.BasicHttpClientConnectionManager.getConnection(BasicHttpClientConnectionManager.java:266)
        at org.apache.http.impl.conn.BasicHttpClientConnectionManager$1.get(BasicHttpClientConnectionManager.java:217)
        at org.apache.http.impl.execchain.MainClientExec.execute(MainClientExec.java:190)
        at org.apache.http.impl.execchain.ProtocolExec.execute(ProtocolExec.java:184)
        at org.apache.http.impl.execchain.RetryExec.execute(RetryExec.java:88)
        at org.apache.http.impl.execchain.RedirectExec.execute(RedirectExec.java:110)
        at org.apache.http.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:184)
        at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
        at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:55)
        at org.jboss.resteasy.client.jaxrs.engines.ApacheHttpClient4Engine.invoke(ApacheHttpClient4Engine.java:313)

So is this not how you should reuse clients, or am I missing something obvious?

Closing the client seems to just result in other exceptions...

Answer 1

Yes, the issue your experiencing definitely related to your pool implementation. I actually see at least 3 problems in your code:

1. SimpleClientResourcePool is not a thread safe. Access to pool collection is not synchronized. In your case the Queue will also be better than LinkedList, so look at ConcurrentLinkedQueue provided by concurrent package.
2. You should withdraw Client object from the pool or move it to another collection of "used clients" depending on your needs before release is not called for it.
3. Always do {} even around one-line if statements. Code will not compile after automatic merge if someone in your team add another lines in that "if".

Applying such advises you can start from something like this:

public class SimpleClientResourcePool implements ResourcePool<Client> {
    private final Queue<Client> pool = new ConcurrentLinkedQueue<>();

    @Override
    public Optional get() {
        return Optional.ofNullable(pool.poll());
    }

    @Override
    public void release(Client value) {
        pool.offer(value);
    }
}

Hope it helps!