Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use same connection pool for all clients per OKHttp recommendations #1274

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,8 @@ after_success:
# infrastructure. This should prevent the buffer overflow from
# https://github.com/travis-ci/travis-ci/issues/5227
sudo: false

# due to travis-ci changes in default build environment,
# this must be specify in order for oraclejdk8 to install
# https://blog.travis-ci.com/2019-04-15-xenial-default-build-environment
dist: trusty
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,10 @@
public final class HttpCommand<R> {

private static final Logger LOG = LoggerFactory.getLogger(HttpCommand.class);
private static OkHttpClient client;

private HttpRequest<R> request;
private OkHttpClient client;
private final HttpRequest<R> request;
private OkHttpClient reqClient;
private Request.Builder clientReq;
private int retries;

Expand All @@ -63,7 +64,15 @@ public static <R> HttpCommand<R> create(HttpRequest<R> request) {
}

private void initialize() {
OkHttpClient.Builder okHttpClientBuilder = new OkHttpClient.Builder();
if (client == null) {
synchronized (HttpCommand.class) {
if (client == null) {
client = getClient(request.getConfig());
}
}
}

OkHttpClient.Builder okHttpClientBuilder = client.newBuilder();
Config config = request.getConfig();

if (config.getProxy() != null) {
Expand Down Expand Up @@ -91,8 +100,8 @@ private void initialize() {
if (HttpLoggingFilter.isLoggingEnabled()) {
okHttpClientBuilder.addInterceptor(new LoggingInterceptor());
}
okHttpClientBuilder.connectionPool(getConnectionPool());
client = okHttpClientBuilder.build();

reqClient = okHttpClientBuilder.build();
clientReq = new Request.Builder();
populateHeaders(request);
populateQueryParams(request);
Expand All @@ -101,15 +110,23 @@ private void initialize() {
/**
* Create ConnectionPool optimized for short lived client with little chance to reuse connections.
*/
private ConnectionPool getConnectionPool() {
private static OkHttpClient getClient(Config config) {
int maxIdleConnections = 0;
// OkHttp creates "OkHttp ConnectionPool" thread per every ConnectionPool created to mange its connections. It
// lives as long as the last connection made through it + its keep alive timeout. By default that it 5 min which
// makes no sense for openstack4j since the connections or pools are not reused (after HttpCommand completion,
// at least). Setting strict keepAlive duration here so the connections and threads does not hang around longer
// than necessary.
int keepAliveDuration = 500;
return new ConnectionPool(maxIdleConnections, keepAliveDuration, TimeUnit.MILLISECONDS);
ConnectionPool cp = new ConnectionPool(maxIdleConnections, keepAliveDuration, TimeUnit.MILLISECONDS);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Author of the original workaround here. I like the approach of rectifying the misuse of OkHttp client, though I belive we have to rethink the keepalive duration and the size of the pool. The only reason this was ever tempered with was the numerous OkHttpClient instances took minutes to purge the threads and connections causing resource waste. I guess we can get back to OkHttp's defaults here as we are using the (single) client the was it was meant to be.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we need any of this now: dc97e67#diff-e08fdcb99d047ca41bbe3fe37e41802f


// OkHttp performs best when you create a single `OkHttpClient` instance and reuse it for all of
// your HTTP calls. This is because each client holds its own connection pool and thread pools.
// Reusing connections and threads reduces latency and saves memory. Conversely, creating a client
// for each request wastes resources on idle pools.
OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder();
clientBuilder.connectionPool(cp).build();
return clientBuilder.build();
}

/**
Expand Down Expand Up @@ -138,7 +155,7 @@ else if(request.hasJson()) {
body = RequestBody.create(null, Util.EMPTY_BYTE_ARRAY);
}
clientReq.method(request.getMethod().name(), body);
Call call = client.newCall(clientReq.build());
Call call = reqClient.newCall(clientReq.build());
return call.execute();
}

Expand Down Expand Up @@ -224,4 +241,4 @@ static class LoggingInterceptor implements Interceptor {
return response;
}
}
}
}