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

Api 1.8.0 leaves some background thread running. #74

Open
kulminaator opened this issue Mar 11, 2017 · 7 comments
Open

Api 1.8.0 leaves some background thread running. #74

kulminaator opened this issue Mar 11, 2017 · 7 comments

Comments

@kulminaator
Copy link

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

chbrown commented Mar 11, 2017

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

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

@chbrown
Copy link
Collaborator

chbrown commented Mar 12, 2017

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

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

chbrown commented Mar 14, 2017

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

bitti commented Jan 21, 2018

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

chbrown commented Jan 21, 2018

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.

rogerallen added a commit to rogerallen/tweegeemee that referenced this issue Feb 24, 2020
- compile brings in explore.clj & runs it.  use (comment...)
- needed to deal with adamwynne/twitter-api#74
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

3 participants