Use same connection pool for all clients per OKHttp recommendations #1274
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I do not think we need any of this now: dc97e67#diff-e08fdcb99d047ca41bbe3fe37e41802f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The OKHttp documentation recommends the following usage pattern:
The current
HttpCommandimplementation for OKHttp creates a newOkHttpClientfor each request, and subsequently a new connection pool for each request. This may not be an big issue since the connection pool is set to allow 0 connections and also to evict connections after 500 ms. Nevertheless, the overhead of initializing a new connection pool and related threads for each and every http request may not be negligible for a large number of http requests.This pull request attempts to address this by providing a singleton client available to all requests. This singleton client is not the client used for the actual http requests, but serves as a "template" client. We use
client.newBuilder()to create a shallow copy of the singleton client. This will reuse the existing connection pool, but still enable us to fully customize each http request.The singleton OkHttpClient will be instantiated in the
initializemethod ofHttpCommand.