square / okhttp

Square’s meticulous HTTP client for the JVM, Android, and GraalVM.
https://square.github.io/okhttp/
Apache License 2.0
45.87k stars 9.16k forks source link

Documentation OkHttp 3: Highlight that one must not recreate the OkHttpClient for each request #2636

Closed kypeli closed 8 years ago

kypeli commented 8 years ago

With OkHttp 3.3.1 if one is recreating the OkHttpClient for each request like so:

OkHttpClient client = new OkHttpClient.Builder().build();

you will run out of file handles for your app. Your application will terminate with the following error message after a while

'FORTIFY_SOURCE: FD_SET: file descriptor >= FD_SETSIZE. Calling abort().'

It is mentioned in the OkHttp 3 release notes that one should avoid recreating the OkHttpClient because the app may run out of memory due to each client getting its own thread pool.

But in my experience, one definitely should not recreate the OkHttpClient object because of the issue with file handles, which is much harder to debug and link to OkHttp 3.

On the other hand, if the OkHttpClient object is going out of scope and being garbage collected by the system (assumably), it may also be a bug that the thread pool is not cleaned and file handles not being closed properly.

swankjesse commented 8 years ago

Wanna send a pull request?

kypeli commented 8 years ago

Looking at your samples from https://github.com/square/okhttp/tree/master/samples it seems that you are creating the OkHttpClient at the same time with the Request sent to the client.

Do you think the samples should be changed too? Based on my experience, it's just a really bad practice to create the OkHttpClient on-demand with the Request. Or do you think it's a bug that the file handlers are not released? Thus, creating the OkHttpClient on-demand should be ok?

kypeli commented 8 years ago

I would probably at least add a new section to your FAQ in the Wiki, if for nothing else, for Google to find it for other people if the stumble on the same error message.

https://github.com/square/okhttp/wiki/FAQs

Error: 'FORTIFY_SOURCE: FD_SET: file descriptor >= FD_SETSIZE. Calling abort().'

If you get this error when calling execute() on your Call, it means that OkHttp has not released the file handles associated with previous HTTP calls and you are running out of file handles allowed for a single Android application.

The reason for this is probably that you are creating a new OkHttpClient object for each of your HTTP requests, which in turn creates thread pools for each client which will leave connections/file handles around.

To solve this issue, create a single static OkHttpClient that is shared between all your Call requests.

English is not my native language, so feel free to edit as seen fit, if you find this useful.

kypeli commented 8 years ago

The bug I encountered is due to the fact that I created a new OkHttpClient for each request and made several successive requests in a tight loop. This together with the fact that the default OkHttpClient.Builder creates a new ConnectionPool for each client and this ConnectionPool will keep all connections around for at least 5 minutes before discarding them as idle. This will make the file handles run out quite fast on Android.

Create a default ConnectionPool with a 5 minute cleanup period: https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/OkHttpClient.java#L390 https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/ConnectionPool.java#L85

This will keep the connections around for at least 5 minutes: https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/ConnectionPool.java#L208

If we don't want to force the user to create a custom ConnectionPool and make sure that file handles are being cleaned, I think this should be handled somehow. I am not (yet) familiar enough with the OkHttp source code to know if this is even feasible to fix, but I will look into a pull request. Input is warmly welcomed to hear if this is feasible to work around or fix inside the OkHttp client.

swankjesse commented 8 years ago

I think we fix this with documentation. Which docs did you read before making that mistake?

kypeli commented 8 years ago

Well, neither the examples at http://square.github.io/okhttp/ nor the Wiki say anything about this corner case. Also the samples in GitHub do show that the client is created together with the request.

If there's documentation about ConnectionPool request cleanup and how not to use the OkHttpClient, I must have missed it.

kypeli commented 8 years ago

I would also like to point out that the comment about OkHttpClient in version 3 being stateless, drew a curveball on me when it comes to this issues, which is why I thought it's even a better approach to create the client with the requests.

jaredculp commented 8 years ago

:+1: to these doc improvements! Just ran into this issue and was able to easily figure it out thanks to these changes.