nickoala / telepot

Python framework for Telegram Bot API
MIT License
2.42k stars 474 forks source link

loop propagation for the underlying session #362

Open xen opened 6 years ago

xen commented 6 years ago

Here is my fix for the #359 What have been done here:

I didn't make any additional tests since I'm working on a prototype of my project. It is a very early stage and I try to make pieces together.

Belanchuk commented 6 years ago

This is works for:

import uvloop
asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())
xen commented 6 years ago

Any chance that this PR will be merged?

nickoala commented 6 years ago

Sorry, I'd say no, mainly because I don't want to take care of one more framework.

Thanks for the work though. It's good for telepot to embrace more situations, but I simply don't have the time and motivation to keep up with those concerns. That being said, I appreciate @xen 's work. Thank you.

xen commented 6 years ago

@nickoala I think we misunderstood each other. Don't let that I mention Sanic confuse you.

My pull request was built around 2 lines of code. Existing code silently create an internal instance of asyncio.loop object and use it for a connection pool. Instead of existing top-level loop instance that already exists when Bot object created (for example here). Actually, I don't know why your code is working. Probably some magic inside asyncio allow mixing different loops instances into one, but I don't know if it will work forever or will be changed in the future. Current asyncio loop approach requires cancerous instantiation of the loop objects everywhere and propagation for the underlying asynchronous code. It looks ugly but allows to control how loop instances are working inside.

You made the shortcut and it works. But there exists another implementation of the loop by Magicstack Inc. called uvloop (https://github.com/MagicStack/uvloop). It is also popular and advertised as a faster alternative for the asyncio.loop. BTW, it made by the team that created asyncio lib itself. But my changes doesn't change existing functionality, at least I never saw any uses of the connection pool in the documentation and examples provided in the repository.

So I made telepot "loop provider independent". As a bonus, this made telepot works inside Sanic, also very popular web framework.

I think it worth to keep. At least @Belanchuk already found me via telegram to ask is my patch working.

nickoala commented 6 years ago

@xen, after reading over your message a few times, I start to understand what you are trying to get at. I did misunderstand your intention a little bit. Your modification did not seem like it was "built around 2 lines of code"! :sweat_smile:

First, let me explain my code a little bit. As you are aware, the module telepot.aio.api has a _loop variable which is shared by all things requiring an event loop in that module. On the other hand, in the module telepot.api, the Bot object also has its own _loop which is, again, shared by all things requiring an event loop there. These two _loops, if not user-specified, use asyncio's default event loop. That is, if the developer does not supply his own loop, the Bot's loop and the telepot.aio.api module's loop are one and the same. That's probably why my code works.

Now, what should I do if I want to substitute my own event loop? I would supply my own loop to the Bot constructor. The existing code does make an effort to sync the two _loop variables, as evidenced here. However, now that I read the code again, I am not sure that line has the desired effect, because telepot.aio.api._pools possibly has been initialized and is still using the original loop. What is needed is an additional function in the module telepot.aio.api that refreshes everything. With such a function in place, supplying a new loop to the Bot constructor should do the job.

Without that function, a workaround may still be possible. According to uvloop's github page, replacing asyncio's default loop with uvloop is easy. How about doing that before loading telepot? For example,

import asyncio
import uvloop
asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())

import telepot.aio

bot = telepot.aio.Bot(...)

By not supplying any loop, the Bot and the module telepot.aio.api simply use asyncio's default loop, which is now uvloop! Would that work?

I will re-open the PR and see what you have to say ...

I also would like to explain a bit why the _pools variable exists, but it's getting off topic for now. I will do that later ...

unittolabs commented 6 years ago

Unfortunately just adding

import asyncio
import uvloop
asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())

import telepot.aio

bot = telepot.aio.Bot(...)

doesn't work at all.

It's very sad because telepot looks like a much stronger solution than aiogram (by the way they also raise the same error). We don't have any other async frameworks for working with telegram for now. Write 2 different services and build some bridge between each other will be stupid here (I think). So I (and not only I) will be very glad for fixing this PR and accepting of it.

@xen @nickoala

nickoala commented 6 years ago

As I have indicated here, I have decided to stop enhancing telepot because I want to spend time pursuing other interests.

That said, I want to re-iterate that I appreciate @xen's initial work to make telepot more inclusive and all of your supports. My apology, again, for not meeting the demand.