Add OkHttpClient#close method #3372
Comments
|
We want to encourage users to share the connection pool and dispatcher between clients. But if you do, you can't just tear everything down. |
|
That's a fair use case. Would it make sense to provide a utility method that correctly cleans up a client (for clients where the lifetime of the client, dispatcher and pool match)? |
|
If you don’t mind, lemme step back a bit. Why do you want to close your OkHttpClient? |
We're using OkHttp in a service environment (i.e. not Android/Mobile). We're using one client with its own connection pool per downstream service. We want to cleanup the resources attached to the client in a couple of places:
|
|
Got it. Creating a new OkHttpClient is relatively expensive and I’m reluctant to make it easy to create and destroy clients because it may encourage inefficient use cases. You’re free to create and destroy clients for each test, but I want you to write the code to do set up and tear down so that you can take responsibility for the performance cost of that. (The performance cost is basically the cost of creating an ExecutorService for the connection pool and another for the Dispatcher. I’ve found in my own server applications that the costs of creating and destroying thread pools is substantial in the overall cost of running the test.) |
I'm not quite sure how making me write code to properly close a client makes me take responsibility for the performance cost of creating new clients all the time. The Javadoc on
and that creating new clients all over the place should be avoided. So IMHO that fact is already clearly communicated. From our own usage I observed that we have utility methods to close an I'll close this. Thanks for your feedback. |
|
For most efficiency, you'll have one ConnectionPool and one Dispatcher in your process. These can be shared by many OkHttpClient instances. If you shut down the Dispatcher, none of the OkHttpClient instances that use it will work. Similarly for shutting down the ConnectionPool. An analogy: imagine unplugging your residential modem and WiFi router when you're done using Firefox. It's easy enough to plug them back in when you need them. And it's handy to know what to shut off if you're not going to use Firefox ever again. But I still think it’d be a misfeature for Firefox to shut down these devices when it exits, or even to offer an option to do so. |
|
Here is another use case. We are using OkHttpClient in a mobile environment, we need to clean up the client any time a user logged out, to make sure we don't retain any keys, sessions, connections when the user is not authenticated. We don't just want a "close() " method, we want a "destroy()" method, so we know its not usable. |
|
The problem with any Instead I recommend closing the three things recommended in the docs: That way if your application has a shared cache but not a shared connection pool or dispatcher you can close all the parts you want to close. |
|
Is that going to be enough to ensure I don't get a memory leak, when I toss the OkHttpClient into the ether? |
|
Mostly. There’s an executor service in the connection pool that stays alive for 60 seconds, so if you’re creating and discarding OkHttpClients more frequently than once-per-minute eventually these will stack up. My recommendation is to create a single OkHttpClient and to either reuse it, or to build derivative OkHttpClient instances from it by calling |
|
Hi. Regarding calling: In my case, I keep connections open for 24 hours (due to some business requirements) Should I wait until all connections are idle to effectively shut down the pool? |
|
Connections currently carrying requests and responses won't be evicted. Everything else will be, whether or not the pool duration has elapsed. |
|
Hey team I went to this issue as I was looking for a way to definitely stop the client and its underlining resources. I'm using 4.12.0. I'm using RandomizedTesting framework for my tests. It detects automatically ZombieThreads: @RunWith(RandomizedRunner.class)
@ThreadLeakScope(ThreadLeakScope.Scope.SUITE)
@ThreadLeakLingering(linger = 1000) // 1s lingering
public class ZombieOkHttpIT {
@Test
public void testZombie() throws Exception {
OkHttpClient httpClient = new OkHttpClient.Builder()
.connectionPool(new ConnectionPool(1, 100, TimeUnit.MILLISECONDS))
.connectTimeout(100, TimeUnit.MILLISECONDS)
.writeTimeout(100, TimeUnit.MILLISECONDS)
.readTimeout(100, TimeUnit.MILLISECONDS)
.callTimeout(100, TimeUnit.MILLISECONDS)
.build();
Request request = new Request.Builder()
.url("http://127.0.0.1:8080")
.build();
try (Response response = httpClient.newCall(request).execute()) {
System.out.println("response = " + response.body().string());
} catch (ConnectException e) {
System.out.println("You need to start a minio service first:");
System.out.println("docker run -p 8080:80 httpd");
}
httpClient.dispatcher().executorService().shutdown();
httpClient.connectionPool().evictAll();
}
}This gives: I tried to use the default client, define some timeouts (as per the shared code) but I can't find the way to properly close everything. I tried what's written in documentation: httpClient.dispatcher().executorService().shutdown();
httpClient.connectionPool().evictAll();But still no luck. Any idea of what should be done to close the Full demo code available at https://github.com/dadoonet/demo-ssh-mino/blob/master/src/test/java/fr/pilato/test/zombie/okhttp/ZombieOkHttpIT.java |
|
There are many issues discussing this in the past (such as this one!). For Okio, the thread is global and shuts itself down automatically after a timeout. See square/okio#738. I can't remember if the task runner is global or per-instance within OkHttp, but I would expect it to behave similarly. |
|
But I tried to add a wait (just for test purposes) and after 1+ minutes threads were still there. |
What kind of issue is this?
already exists! Don’t send pull requests to implement new features without first getting our
support. Sometimes we leave features out on purpose to keep the project small.
We have been writing helper methods to properly close/shutdown an
OkHttpClient.The Javadoc on the
OkHttpClientdescribes how to do this but ideally there is aclose()method onOkHttpClientthat does this correctly (additionallyOkHttpClientshould probably be ajava.io.Closeableas well).Are there any reasons there isn't? Otherwise I'd be happy to raise a PR adding this.
The text was updated successfully, but these errors were encountered: