search

adamwynne / twitter-api

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

file descriptors leak #75

Open danielsz opened 7 years ago

danielsz commented 7 years ago

Calling make-oauth-creds with the first supported arity, that is with app-key and app-secret only, will leave open files. The offending function is request-app-only-token, with further calls to execute-request-callbacks and callbacks-sync-single-default. This smells like threads are left waiting for I/O somewhere down the stack instead of being shut down.

https://github.com/adamwynne/twitter-api/blob/070063ff3d159bb27d470b795b0ca881c0d24b48/src/twitter/oauth.clj#L50

The leak can be substantial when calling make-oauth-creds often under that arity.

sudo ls -l /proc/<pid>/fd | wc -l
chbrown commented 7 years ago

I didn't try to replicate this, but I'm pretty confident it's due to the unmanaged (create-client)call in that function, which ought to be wrapped in a (with-open [... (create-client)] ...) or otherwise .close'd manually.

I was finding client/connection handling tricky in that it's hard to support (in the API design) letting the user manage clients but also not force them to take on that complexity if they don't want. This particular call could be fixed pretty easily, and I'd merge that PR. But the need for a sanely managed client pool becomes more of a problem when you're trying to make the most of the async nature of this library, especially with long-running connections to the streaming endpoints.

I might recommend taking a look at my fork, which I'm calling twttr. twttr uses the (more) actively maintained aleph instead of http.async.client for its HTTP needs. I was intending the http.async.client -> aleph transition to constitute a major version bump (v2) for this library, but otherwise be backward-compatible.

But there were a number of design conflicts that pushed aleph integration further and further from being able to support the twitter-api v0 / v1 API. E.g., http.async.client is callback oriented, but aleph handles long-running connections with its manifold companion library via manifold.stream.

So twttr is more of a paradigm shift than a major version bump, and involves a lot of other API changes. YMMV depending on your current investment in the twitter-api v1 public API.

danielsz commented 7 years ago

Thank you for taking the time to explain, and for the pointer to twttr. That project looks interesting and I hope to be able to take a closer look soon.

For now, I am memoizing the offending call, which takes care of the leak. Admittedly, this is nothing more than a kludge (but good enough for me at this point).