fbchat-dev / fbchat

Facebook Chat (Messenger) for Python
https://fbchat.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.19k stars 412 forks source link

asyncio fbchat #411

Open jedi7 opened 5 years ago

jedi7 commented 5 years ago

Description

Hi, do you have a plan to use asyncio in your project? If yes, I can try to do some work. If no, is ok for you to use part of your code in another project?

Thanks Jarek

madsmtm commented 5 years ago

We could add asyncio support, but then we'll be limited to their paradigm. I'd rather add more general async/await support, so that we can support both asyncio, trio, curio, synchronous code and so forth, but we're waiting on https://github.com/urllib3/urllib3/issues/1323 before that's going to happen. 😕

But you're free to do basically anything with the code, as long as you reference back to this project. See the license for more info. And if you do experiment with it, I'd love to see what you come up with! 😊

tulir commented 5 years ago

I've made an asyncio fork: https://github.com/tulir/fbchat-asyncio

vincent-lg commented 5 years ago

Hi,

Just want to add some feedback here.

asyncio support is not just neat, it's important for the future. Asyncio has been there since Python 3.5 and is considered mostly stable from Python 3.7 and up as far as I know. Adding asuncio support would make a huge difference for these versions (no need to worry about things like parallelism and concurrency, at least on paper). I'll strongly recommend adopting a course for this feature, knowing that urllib3 support shouldn't be an issue: a lot of other asyncio libraries exist for that, among which is aiohttp which I find pretty straightforward to use. Unfortunately, adding asynchronous support in fbchat might require adding an intermediary layer, as the support for await/async keywords would mean using other functions. I would recommend using a different endpoint (say, AsyncFacebook) and then use the same naming convention (await chat.send(...) for instance (please don't add asyncSend equivalent to the same endpoint, that would only confuse users and make the API less straightforward to use).

I believe this is a major feature. Admittedly, Python users still playing with Python prior than 3.5 won't benefit from it as much, but the population of users playing with Python 2 and Python <3.5 isn't increasing by all means.

Thanks for your great work! And all the best,

madsmtm commented 5 years ago

Thanks for your input, @vincent-lg!

Regarding Python 2.7 users, then I'm currently in the process of developing v2, where we'll only be supporting Python 3.5+, so that's not really a problem (Oh, and don't worry, if we were to add support, it wouldn't be the ugly asyncSend or whatever 😉).

And luckily, I've recently spent a lot of time refactoring, so that exactly this use-case can become more feasible - we were previously relying on a lot of requests specific features, but now it should be possible to move to aiohttp/urllib3!

However, I'd like to reiterate, I'd rather add more general async/await support. I personally disagree with the choices made in asyncio/aiohttp, so I don't want to convert this project to work with that (so, not a technical choice, it's definitely possible, but I don't want to put in the effort 😕).

That said, I'm open to other, new possibilities, I've been dreaming of using fbchat with trio for months now!

madsmtm commented 4 years ago

Okay, a few months have passed, and I'm starting to realise that the linked issue (now called hip) won't happen for quite some time; and even if it does, it dawns to me that we can add support for asyncio now, and then add support for trio and others later.

But I'm not very experienced in asyncio, so I'll need all the help I can get!

A few questions:

  1. I see that a lot of people use (contrived example):

    import asyncio
    async def some_function(time, loop=None):
        loop = loop or asyncio.get_event_loop()
        await asyncio.sleep(time, loop=loop)

    Do we really need to pass a loop variable to every function? Can't we just do the following:

    import asyncio
    async def some_function(time):
        loop = asyncio.get_event_loop()
        await asyncio.sleep(time, loop=loop)

    ? (Bear in mind that we support Python 3.5, so if it's something specific to that, let me know!)

  2. Is aiohttp the HTTP library to use, or are there better alternatives? Keep in mind I'm aiming for maturity here 😉

  3. I see that you, @tulir, are using your own fork of hbmqtt, to support sending usernames. Do you plan on getting that fixed upstream? Otherwise, does anyone know of an alternative? Or do we have to retort to some manual asyncio code with paho.mqtt.python.

  4. I see that you've done other things in your branch other than adding asyncio, @tulir. Do you perhaps want to contribute that upstream?

jedi7 commented 4 years ago

regarding the 1) question. loop variable is not needed.

example:

async def other_foo():
  await fetch_data()  # start fetching data and switch context to another await thread (but there is none)

async def test_connection():
  await other_foo()
  # data fetched

def main():
    loop = asyncio.get_event_loop()
    loop.run_until_complete(test_connection())

Simply every "await" will do context switch to another task (if any) Another task is created by "create_task". I hope it helped a little

madsmtm commented 4 years ago

That was my suspicion as well, thanks for the clarification 👍

tulir commented 4 years ago
  1. It's not needed, asyncio is moving away from passing the loop parameter everywhere. In fact, you'll find many methods (including sleep) have a notice like this:

    Deprecated since version 3.8, will be removed in version 3.10: The loop parameter.

    It should work fine without it in older versions too.

  2. aiohttp is pretty much the de facto HTTP library. There are some other options for servers, but not many for clients.
  3. Those changes are fairly simple, I can make some PRs to the upstream library. It's mostly the MQTT version headers (actual changes between 3.1.0 and 3.1.1 are minimal) and passing complicated usernames with special characters (the library does support usernames, but only by parsing the url).
  4. Those changes are a bit more hacky, might be better to redo them properly
    • Changed the logging to pass loggers as parameters instead of using a hardcoded one. My program supports an arbitrary number of facebook clients, so logging really needs to identify which one it's for.
    • Re-added internal FBchatPleaseRefresh handling because it seems silly to have to handle them separately in every single call when the handling is exactly the same.
madsmtm commented 4 years ago

I'd be very nice if you would add the changes to the MQTT library upstream, would save me a lot of trouble!

Regarding logging, I like the idea of logging the current user, though I don't like passing loggers as parameters; rather, I'd have that handled internally, and perhaps let the advanced logging user differentiate between the logging items differently (using a custom logging.Logger.makeRecord?).

And yeah, perhaps FBchatPleaseRefresh should be handled internally, it was mostly because I knew we couldn't handle everything (e.g. FBChatNotLoggedIn), so I thought the user would have to write some error handling no matter what. But perhaps it'd be better to allow the user to register an error handler on the Session class (previously called State)?

Thanks for your help!

tulir commented 4 years ago

My fork has some issues with the mqtt connection (it stops receiving updates after a few days), so I decided to rebase it to the latest upstream version (i.e. take the latest version and asyncify it from scratch) and see if that works better.

I think I have paho.mqtt working with asyncio without too many manual asyncio hacks, it's mostly setting a few callbacks to set up the basic IO and calling mqtt.loop_misc in a loop.