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

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

Closed
kypeli opened this issue Jun 17, 2016 · 8 comments
Labels
documentation Documentation task
Milestone

Comments

@kypeli
Copy link

kypeli commented Jun 17, 2016

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
Copy link
Member

Wanna send a pull request?

@kypeli
Copy link
Author

kypeli commented Jun 18, 2016

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
Copy link
Author

kypeli commented Jun 18, 2016

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
Copy link
Author

kypeli commented Jun 19, 2016

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
Copy link
Member

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

@kypeli
Copy link
Author

kypeli commented Jun 19, 2016

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
Copy link
Author

kypeli commented Jun 20, 2016

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.

@swankjesse swankjesse added the documentation Documentation task label Jun 25, 2016
@swankjesse swankjesse added this to the 3.4 milestone Jun 25, 2016
swankjesse added a commit that referenced this issue Jul 9, 2016
@jaredculp
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation task
Projects
None yet
Development

No branches or pull requests

3 participants