Removed GLib dependency

[blacklight]

This replaces the GLib dependency (used to handle connection events) with asyncio.

Context: #71

TODO: Fix tests

[adamcik]

If this is setting the loop globally then it is a no go, we can't have random extensions fighting over who controls this, in that case it would need to be mopidy itself that owns and starts the loop for all extensions.

[blacklight]

@adamcik I've opened mopidy/mopidy#2197 on mopidy to initialize the loop when the application starts.

Btw there was already a new_event_loop/set_event_loop call in the http module (needed for Tornado I guess).

[kingosticks]

The comment in the http case explains it's per thread. Presumably that's right and what you had was ok but I've not checked their docs.

[blacklight]

I think this should be the case indeed. Just done a quick test:

import asyncio
import threading


async def my_coroutine():
    print("Coroutine:", id(threading.current_thread()))
    await asyncio.sleep(1)


def my_thread():
    loop = asyncio.new_event_loop()
    asyncio.set_event_loop(loop)
    loop.run_until_complete(my_coroutine())
    print(f"Closing loop {id(loop)} from {id(threading.current_thread())}")
    loop.close()


def main():
    print("Main:", id(threading.current_thread()))
    loop = asyncio.new_event_loop()
    asyncio.set_event_loop(loop)

    thread = threading.Thread(target=my_thread)
    thread.start()
    loop.run_until_complete(my_coroutine())
    thread.join()

    print(f"Closing loop {id(loop)} from {id(threading.current_thread())}")
    loop.close()


if __name__ == "__main__":
    main()

Output:

Main: 135176582243936
Coroutine: 135176582243936
Coroutine: 135176571626432
Closing loop 135176571884624 from 135176571626432
Closing loop 135176571625424 from 135176582243936

So calls to asyncio.new_event_loop performed in different threads result in the creation of different loops, and as long as set_event_loop is called at most once in each thread things should work.

So as long as we expect new_event_loop+set_event_loop to be called at most once per pykka.ThreadedActor is it fine to let extensions manage their own loops?

If so I can close the PR on mopidy.

[adamcik]

I honestly didn't remember if this is process or thread wide, so the comment was meant as a question not a statement of fact.

So if this actually thread wide just following the same pattern as the HTTP extension and creating our own thread for the loop could make sense. Then each exentsion can have thread dedicated to running it's own asyncio loop and dispatching to/from pykka? The downside is of course having a thread per loop, but given how many threads we already have floating around for pykka I doubt this makes a large difference in practive.

The other alternative is to have a sentral thread in mopidy that starts when an exentsion (or something else in mopidy) requests for something to run in an event loop, or requests the loop. The pro of this would be having a single loop and fever threads, and extensions have less things to manage and get right. While the negative would be that if anyone accidentaly does blocking work in something they run on the loop it blocks everyone. If each extension has it's own loop we have more isolation.

I have not idea which one would actually be nicer or better for Mopidy's needs, but those are the two variants and some of the tradeoffs that came to mind off the top of my head.

[akx]

I'm wondering... why go asyncio in the first place? Wouldn't plain threading do?

[blacklight]

@akx my main concern is the blocking nature of send/recv on sockets, which may block the whole process even if running in separate threads. Plus, my ultimate plan here would be to replace the current LineProtocol + Connection + Server implementation with a single class that extends asyncio.Protocol, which should greatly simplify the implementation.

[kingosticks]

For io work like this, threads are fine (and a well trodden path in Mopidy so maybe a good choice for a quick fix here until that other change happens?)

[akx]

which may block the whole process even if running in separate threads.

Surely not... how would that happen? 🤔

[adamcik]

which may block the whole process even if running in separate threads.

Surely not... how would that happen? 🤔

The case that comes to mind would be a plugin using CPython and forgetting to release the GIL while waiting for something outside of python code, but that is not the case here If we have a dedicate thread that uses epoll, select, ... under the hood to wait for sockets to have data and then dispatches things to pykka for processing that should be fine. Irrespective of if we use something prebuilt or do it "low level" (at least by python standards).

[blacklight]

in that case it would need to be mopidy itself that owns and starts the loop for all extensions.

That's a valid point - the loop should be created only once and before all extensions are initialized. Especially if actors run in separate threads.

I can open a PR on mopidy for that. Just let me know what's the preferred approach here. From what I'm reading the options are:

1. We replace GLib events with asyncio, with the loop initialized at application start. This would involve:
   1.1. Adding the loop initialization logic in mopidy itself.
   1.2. Changing the tests - which should be done anyway to remove the GLib dependencies, and I'm already in the process of migrating.
2. We move the current Connection implementation to a separate thread - but that would also require implementing synchronization mechanisms and timeouts, since those are currently managed by GLib anyway.

I'm slightly more in favour of option 1 because on the long run it may allow protocol handling to be simplified on many other extensions too - plus the next step could be to just drop all the Server+Connection+LineProtocol constructs and move all the implementation in a class that extends asyncio.Protocol.

[adamcik]

Note that I don't think pykka supports asyncio yet, but you can probably workaronud that. jodal/pykka#99 (comment) might be relevant as well.

[blacklight]

Note that I don't think pykka supports asyncio yet

I've tested the implementation in this PR both with mpc and ncmpcpp and it seems to work fine.

Probably asyncio.run_coroutine_threadsafe and loop.call_soon_threadsafe also do the job to prevent threaded actors and asyncio primitives from stepping on each other's toes - but maybe run_in_executor can be more efficient, with a pool of threads it could even handle more tasks in parallel.

[jodal]

Pykka is still entirely ignorant of asyncio, for exactly the same reasons as five years ago in the mentioned PR: I have never thought through how Pykka with threads+asyncio should work in its entirety. There was someone that ported all of Pykka to asyncio, but that gives us no migration story from threads to asyncio, and I suspect that keeping different extensions in different threads, even after the introduction of asyncio inside some extensions, might be good for our mental health.

Tl;dr: I'd love for Pykka to have a good story about asyncio interop, but I don't have a vision developed.

(Also, there is Trio, which shouldn't be ignored. Maybe it should be preferred?)

[blacklight]

(Also, there is Trio, which shouldn't be ignored. Maybe it should be preferred?)

That looks very interesting, but from what I see it's still a layer on top of asyncio which masks all the loop management internals?

In the meantime I have made some progress with the tests - network.test_connection was the biggest suite to migrate as it was tightly coupled with the GLib implementation.

All those tests are now green, the others should hopefully be easier to fix - I've also taken the liberty to remove some tests tied to [enable|disable]_[recv|send], lock acquisition and other mechanisms coupled to the previous GLib implementation, let me know if some of them are worth keeping.

I've also done a few additional stress tests of this branch with a few MPD clients and it seems that everything is working fine.

[jodal]

Re Trio, yes it is very related to asyncio. I mostly mentioned it as yet another thing I need to understand well enough if moving Pykka forwards towards asyncio, so that Trio isn't left out in the cold because on some bad design on my part.

[adamcik]

As for alternatives, https://www.tornadoweb.org/en/stable/tcpserver.html also exists. But if anything it might be more interesting to replace torando with something from the standard library if what is there is good enough, or something more modern. But that should stay out of scope for this PR, unless it makes sense to use it for MPD connection handling of course.

[blacklight]

After a bit of debugging I've realized that the reason why most of the tests are now failing is that the send_request method relies on session.on_receive, which calls send_lines, which is now async.

But no loop is running in the tests because that's started in the MpdFrontend actor itself.

@adamcik @jodal I'm open to suggestions on how to address this without having to rewrite all the tests. It sounds like tests should probably start the loop before execution, maybe in a separate thread?

[adamcik]

I've not looked at this code again before writing this comment, so take this with a grain of salt. This sounds like we could have a pytest fixture that manages the loop stuff, possibly using pytest asyncio. Alternatively using mocks/patching things might work if you hook in at the right level, as async in tests can be rather annoying and lead to flaky tests or other challenges.

[jodal]

Same disclaimer, I haven't looked at the code: I'd probably go all in on pytest-asyncio.

[kingosticks]

Agree. There's enough async footguns out there and no prizes for finding them. Use something off the shelf where possible.