adamwynne / twitter-api

Async io interface to all the twitter APIs
372 stars 64 forks source link

Api 1.8.0 leaves some background thread running. #74

Open kulminaator opened 7 years ago

kulminaator commented 7 years ago

I have created a very simple piece of code, pretty much following the example

(def my-creds (twauth/make-oauth-creds oauth-consumer
                                oauth-consumer-secret 
                                oauth-token
                                oauth-token-secret))

(defn -main
  "I don't do a whole lot ... yet."
  [& args]

  (println "Hello, World!")
  (println (twapi/users-show :oauth-creds my-creds :params {:screen-name "AdamJWynne"}))
  (println "wat")
  )

Sure enough it works until printing the output of users-show. Then emits the expected "wat" ... and then there is silence, the application won't exit. If i comment the users-show out - the application does a clean shutdown. Is there some kind of magic hook that i have to pull? Looks like something is left hanging and jvm figures there is some thread around doing important work still.

chbrown commented 7 years ago

It's most likely the http.async.client library leaving around an open client / channel. My repo is currently an unstaged WIP mess so I haven't tested the following, but try something like:

(ns user
  (:require [http.async.client :as http]
            [twitter.api :refer :all]))

(def creds nil) ; TODO: fill this in

(defn -main
  [& args]
  (println "Starting")
  (with-open [client (http/create-client)]
    (println (users-show :client client :oauth-creds creds :params {:screen-name "AdamJWynne"})))
  (println "Done"))

Or to avoid the client bookkeeping, just go back to using the default memoized client (by supplied no :client argument), and force a full shutdown when you're done with (System/exit 0) at the end of your -main function. (That's what I'd do.)

kulminaator commented 7 years ago

Thanks for the advice, indeed by feeding in the client myself i have control. Good enough for now but looks a tad cumbersome.

chbrown commented 7 years ago

You might also be able to call (http/close (twitter.core/default-client)) manually, at the end, but that seems less Clojurish.

See also discussion. Particularly this in the old http.async.client docs.

Do you have any recommendations on less cumbersome solutions that don't require one opened-and-closed client per API call? Or maybe that's the better solution -- create and close a new client on each call, if no client is explicitly supplied. I suspect that's not as efficient, though.

kulminaator commented 7 years ago

Well in clojure i expect everything to be as side effect free and clean as possible. Messing up one's threading by default is one very proper side effect :( Maybe this is just me. But yes i would expect the default behavior to be clean with a new client every time and optional possibility to feed in a client that is kept alive. Maybe it's better if some other people chip in on the opinion as well.

chbrown commented 7 years ago

expect everything to be as side effect free and clean as possible.

That's a very good point. I think you're right about creating a new client and then closing it ASAP.

I'm planning a huge backward-breaking fork (or maybe a v2, we'll see) in the diy-callbacks branch. In that API, each endpoint function call takes the user/app credentials as a first required argument. (This is probably how twitter-api would have been designed originally, except that the Twitter API once exposed some endpoints that did not require credentials.) My current Credentials protocol could be generalized into a Connection protocol which could provide both the required credentials and an optional client. Though that may not be that much different than the current practice of supplying a :client argument or not.

I'll try to do some benchmarking to see how costly it is to create a new client for each call. FYI that memoized client has been in the library for 6+ years, though it may have behaved differently with earlier versions of the http.async.client library.

bitti commented 6 years ago

I stumbled over the same issue. It would be good if the (http/close (twitter.core/default-client)) trick would be documented somewhere. I don't like to call System/exit since that would kill the repl when I call the -main function from there (and makes it also more complicated to unit test it).

Not all calls seem to be affected by this though. E.g. twitter.api.restful/statuses-update isn't but twitter.api.restful/account-verify-credentials is.

Another workaround seems to be to exclude io.netty/netty from the project dependencies explicitely since then com.ning.http.client.providers.jdk.JDKAsyncHttpProvider instead of com.ning.http.client.providers.netty.NettyAsyncHttpProvider is used as a fallback, but I don't know if that would be an advisable solution.

chbrown commented 6 years ago

Good point @bitti, I added a synopsis of the issue to the README: https://github.com/adamwynne/twitter-api#notes-on-making-api-calls

In the meantime, I'm leaving this issue open because it'd be better if it didn't require the extra bookkeeping in the first place.