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

file descriptors leak #75

Open
danielsz opened this issue Jun 10, 2017 · 2 comments
Open

file descriptors leak #75

danielsz opened this issue Jun 10, 2017 · 2 comments

Comments

@danielsz
Copy link

danielsz commented Jun 10, 2017

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.

(defn request-app-only-token

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

sudo ls -l /proc/<pid>/fd | wc -l
@chbrown
Copy link
Collaborator

chbrown commented Jun 13, 2017

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

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).

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