search

RutledgePaulV / clj-okhttp

MIT License
4 stars 1 forks source link

Responses that return an InputStream are not closed #4

Open vincentjames501 opened 2 years ago

vincentjames501 commented 2 years ago

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.

RutledgePaulV commented 2 years ago

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 commented 2 years ago

@RutledgePaulV , I posted my thoughts here: https://github.com/RutledgePaulV/clj-okhttp/issues/3#issuecomment-1030745224