Skip to content
This repository has been archived by the owner on Jun 2, 2019. It is now read-only.

why aren't we using Keep-Alive for connection to bot api ? #67

Closed
zumoshi opened this issue Apr 26, 2016 · 5 comments
Closed

why aren't we using Keep-Alive for connection to bot api ? #67

zumoshi opened this issue Apr 26, 2016 · 5 comments

Comments

@zumoshi
Copy link
Contributor

zumoshi commented Apr 26, 2016

Hello,
so i was running the tests for telepot and i noticed each message closes the connection and makes a new one ...
by changing the following lines:

...
def _post(url, **kwargs):
    response = requests.post(url, **kwargs)
...

to

...
s = requests.session()
def _post(url, **kwargs):
    response = s.post(url, **kwargs)
...

the tests run at least 5 times faster with only 2-3 connections for 35 messages.
of course we need a better implantation than the patch provided above but i was under impression that we are already using keepalive connections, why aren't we?

@nickoala
Copy link
Owner

Well, I never worried that much about speed, as long as it works, and the code is simple. Now that you bring it up, ok, I will incorporate that into an upcoming version. 😄 Thank you. You are really making telepot that much better.

@zumoshi
Copy link
Contributor Author

zumoshi commented Apr 26, 2016

i currently have over 300 bot tokens on a single application, with single bots that have over 1000 users, so speed is kind of a concern for me.

it would be nice if you could make it so session object would be shared for all threads. as per this issue although its not considered thread safe in all cases but a for single host case without use of cookies is considered safe to use it across threads. specially since according to my understanding delegator bot creates a new thread for each user_id, i don't want that many open connections.

and thanks for considering this, and putting so much effort in making telepot great :)

side note: it is suggestion in the same issue that retry be used in case pool connections (perhaps left open since last request which was a long time ago) may not be correctly marked as closed and fail. maybe we should also incorporate retrying requests with this ?

@nickoala
Copy link
Owner

nickoala commented May 1, 2016

Regarding whether the Session object is thread-safe ...

The issue and comments that you referred to was from 2 years ago. And a lot of recent issues still question requests's thread-safety:

Not reassuring, to say the least. Please don't try to convince me under what conditions it might be thread-safe or not, or how it "seems ok in practice". On thread-safety, I don't take chances.

The most consistent advice the author can give is to use "one session per thread". I am not going to do one session per thread in telepot. We are talking to the same host all the time; all threads should share the same connection pool.

This investigation led me to urllib3, which is more lightweight, has connection pool, and has a thread-safety guarantee from the author. I am going to move to urllib3 for all HTTP requests, except for downloading files. Because streaming download is more easily done in requests, I will keep that part using requests. For all other HTTP connections, I am going to use urllib3.

For telepot's async version, I am also going to incorporate connection pool. That part is easier because of no thread-safety concerns.

Expect connection pooling along with routing features on or before May 17th, if no unexpected obstacle occurs.


I remember retry. It is low-priority, and will probably be done in June or July. No guarantee.

@nickoala
Copy link
Owner

Connection pooling implemented in 8.0

@nickoala
Copy link
Owner

Forgot to mention .... I divide the requests using the following policy:

  1. Short requests, e.g. sendMessage, share one connection pool.
  2. Requests that involve uploading a file use separate connections. I don't want them to occupy connections in the pool.

You may change the policy by overriding the function telepot.api._which_pool, the dictionary _pools, and the tuple _onetime_pool_spec in the same module.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants