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
Connection Pooling #51
Conversation
routemaster/feeds.py
Outdated
@@ -20,6 +21,11 @@ class FeedNotFetched(Exception): | |||
pass | |||
|
|||
|
|||
@functools.lru_cache() | |||
def _get_feed_session(): | |||
return requests.Session() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These aren't thread-safe: I think this should be thread-local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
routemaster/feeds.py
Outdated
@@ -20,6 +21,11 @@ class FeedNotFetched(Exception): | |||
pass | |||
|
|||
|
|||
@functools.lru_cache() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an explicit maxsize
would be nice here, just saves a google to figure out what the connection pool size is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this to a thread local anyway, but unfortunately this probably wouldn't work as there's another layer of indirection within requests to the actual connection pool. There doesn't seem to be an easy way to expose the actual connection pool parameters here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A comment about requests.Session
using urrlib3's underlying connection pool would be great, as I wasn't aware of that!
This adds connection pooling via
requests.Session
to feeds and webhooks.