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

loop propagation for the underlying session #362

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xen
Copy link

@xen xen commented Jan 21, 2018

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

  • one loop object for Bot class and for underlying sessions.
  • removed _pool. I don't know what is the idea behind, so I reimplemented idea as self.session. I know that I probably lack some understanding why it has been done this way before.
  • I left some functions inside telepot.aio.api module, but I don't think it is worth to left it since most functions logically are part of the Bot class.

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.

@xen xen changed the title issue #359 loop propagation for the underlying session Jan 21, 2018
@Belanchuk
Copy link

This is works for:

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

@xen
Copy link
Author

xen commented Mar 6, 2018

Any chance that this PR will be merged?

@nickoala
Copy link
Owner

nickoala commented Mar 7, 2018

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.

@nickoala nickoala closed this Mar 7, 2018
@xen
Copy link
Author

xen commented Mar 7, 2018

@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
Copy link
Owner

nickoala commented Mar 7, 2018

@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"! 😅

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

@nickoala nickoala reopened this Mar 7, 2018
@unittolabs
Copy link

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

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.

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

Successfully merging this pull request may close these issues.

None yet

4 participants