mopidy-mpd
mopidy-mpd copied to clipboard
mopidy
Removed GLib dependency
This replaces the GLib dependency (used to handle connection events) with asyncio.
Context: https://github.com/mopidy/mopidy-mpd/issues/71
TODO: Fix tests
I'm wondering... why go asyncio in the first place? Wouldn't plain threading do?
@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.
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?)
which may block the whole process even if running in separate threads.
Surely not... how would that happen? 🤔
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).
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:
- 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. - We move the current
Connectionimplementation 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.
Note that I don't think pykka supports asyncio yet, but you can probably workaronud that. https://github.com/jodal/pykka/issues/99#issuecomment-714295251 might be relevant as well.
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.
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?)
(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.
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.
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.
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?
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.
Same disclaimer, I haven't looked at the code: I'd probably go all in on pytest-asyncio.
Agree. There's enough async footguns out there and no prizes for finding them. Use something off the shelf where possible.