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

Responses that return an InputStream are not closed #4

Open
vincentjames501 opened this issue Feb 3, 2022 · 2 comments
Open

Responses that return an InputStream are not closed #4

vincentjames501 opened this issue Feb 3, 2022 · 2 comments

Comments

@vincentjames501
Copy link
Contributor

If I use the client in an idiomatic way (or at least in my mind) such as just getting the status to a GET request to https://google.com :

(require '[clj-okhttp.core :as http])
(import java.util.concurrent.TimeUnit)
(def c (http/create-client {:connection-pool             {:max-idle-connections 1
                                                                                        :keep-alive-duration  5
                                                                                         :time-unit            TimeUnit/SECONDS}}))
(println (:status (http/get c "https://google.com")))

If you now wait about 5 seconds, you'll eventually get:

Feb 03, 2022 1:57:15 PM okhttp3.internal.platform.Platform log
WARNING: A connection to https://www.google.com/ was leaked. Did you forget to close a response body? To see where this was allocated, set the OkHttpClient logger level to FINE: Logger.getLogger(OkHttpClient.class.getName()).setLevel(Level.FINE);

If we made the default of #3 to parse as text, this likely wouldn't be a problem. We'd just need to document that if you are coercing the body to an input stream, the caller is responsible for properly closing it when they are done with it.

vincentjames501 pushed a commit to vincentjames501/clj-okhttp that referenced this issue Feb 4, 2022
- Also use :text when not explicitly given for RutledgePaulV#4 and RutledgePaulV#3
@RutledgePaulV
Copy link
Owner

I agree that support for text/* will lessen the likelihood of people running into this for simple use cases. I hope that most people know that when they are handed a stream it is their responsibility to close it, but we can certainly document that fact too. :)

@vincentjames501
Copy link
Contributor Author

@RutledgePaulV , I posted my thoughts here: #3 (comment)

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

No branches or pull requests

2 participants