Opened 6 months ago

Last modified 4 months ago

#3207 new feature-request

Core: migrate from Twisted to Asyncio

Reported by: andar Owned by: andar
Priority: major Milestone: 2.x
Component: Core Version: develop (git)
Keywords: Cc:

Description

We should migrate away from the use of Twisted within Deluge, starting with the Core. asyncio[0] is available as a standard library with the async/await keywords syntax as of Python 3.5. The move to asyncio will help simplify and modernize the code-base, and also allow us to remove a major dependency.

There shouldn't be anything Twisted specific that we actually need over what is provided within asyncio, so it should be a fairly straight-forward conversion.

A small example of how the code would differ: https://asyncio.readthedocs.io/en/latest/twisted.html

Some thoughts:

  • The async/await keywords were added in Python 3.5 which means we would drop support for older versions of Python, I'd even suggest bumping the requirement to 3.6 so that we can use format strings. In my opinion, support for Python 2 should be dropped in the develop branch and for Deluge 2.0 to simplify the code and reduce the maintenance cost.
  • This ticket only addresses moving the Core to asyncio for the time-being. I think this would be a first good step with the UIs to follow. Some care will need to be made to make sure that the DelugeTransferProtocol? is available to both core and UI as it currently stands and that plugins are also unaffected. I believe I should be able to do this in a way that would allow the UIs to continue to use Twisted with a migrated Core.
  • We may have to add an additional dev dependency for asynctest[1] as the standard library does not have some helpful additions this library provides.

Any objections? Anything I didn't think about that may cause a problem?

[0] - https://docs.python.org/3/library/asyncio.html

[1] - https://github.com/Martiusweb/asynctest

Change History (10)

comment:1 Changed 6 months ago by andar

  • Milestone changed from needs verified to 2.0.0

comment:2 Changed 6 months ago by Cas

Yeah I agree moving from Twisted would be useful in the long term. It's a lot of work though as it's integrated into everything, how much do we gain from asyncio?

I don't think moving to Python 3.6 for f-strings is worth the hassle, users likely still need 3.5. I have been targetting minimum Python 3.4 but it is EOL soon so 3.5 is fine and almost there to deprecate/drop Python 2.7.

I am looking to release 2.0.0 soon so this ticket should be for a future milestone. However if we can keep backward compatibility it shouldn't be an issue for future point releases.

Some questions that come to mind:

  • We will need to replace httpdownloader with perhaps requests lib?
  • What will we replace web server with?
  • How will we integrate asyncio with GTK3? What about Windows?

comment:3 Changed 6 months ago by andar

IMHO, the biggest gain from using asyncio over Twisted is more readable code. The use of deferreds and the nested functions all over the place would go away. Using the await keyword makes things read more like synchronous code.

In regards to the amount of work, I don't think it will be that bad for the Core. It will mostly just converting functions to coroutines and replacing d.addCallbacks, etc.. with awaits. The UIs could continue to use Twisted if it will be too much work to convert them.

Twisted is overkill for what we actually need in Deluge, it just happened to be the only library at the time to offer the things we did need.

We could use aiohttp[0] for both httpdownloader and the web server.

For GTK3, we could use gbulb[1], but I'm not sure of the Windows support. If we need to stay with Twisted for GTK ui, that's OK, but it'd be nice to convert at some point. We should also consider that the future of Deluge may be just a new Web UI.

[0] - https://aiohttp.readthedocs.io/en/stable/

[1] - https://github.com/nathan-hoad/gbulb

comment:4 Changed 6 months ago by andar

  • Milestone changed from 2.0.0 to 2.x

comment:5 Changed 5 months ago by JohnDoee

This strikes me as basically a needless rewrite of Deluge.

First thing first, you can use asyncio (and all it brings with it) in Twisted by installing the asyncio reactor. See the examples here of how Django Channels does it here:

https://github.com/django/daphne/blob/e93643ff5a2797f05e88bc59800d7b4dbf41765d/daphne/server.py#L1-L19

https://twistedmatrix.com/documents/current/api/twisted.internet.asyncioreactor.html

The example you use to show how the code would differ tries specifically to be obtuse as they specify keyword equivalency and then doesn't use them equally (i.e. they skip @inlineCallbacks as that'd make the code look VERY similar while saying async def equals it).

Using inlineCallbacks has been part of Twisted for 12-14 years, which would make it predate Deluge? The choice of not using it everywhere must be a design choice?

Not sure how Twisted is "overkill" in any way. Debian stripped it down into smaller packages as it's just a small core and a large bunch of protocols.

To end this, I'm annoyed because it'll break my own plugin while only bringing maybe 5MB of saved space.

Feel free to change reactor but PLEASE abandon the removal of Twisted.

comment:6 Changed 5 months ago by andar

I would argue that Deluge is in dire need of modernization and a rewrite of some of the core code is warranted. Unfortunately there will be some compatibility breaks, but they shouldn't be too difficult to fix.

The advantage really isn't in the space savings, but rather the next steps after the move to asyncio. The use of asyncio and the new keywords makes the code much easier to reason about and will help greatly with the necessary refactoring and updating of the code post-migration.

I'm curious, does your plugin use Twisted extensively? Would you mind pointing me at the code so I can have a better understanding of the burden I'd be putting on you?

comment:7 Changed 5 months ago by JohnDoee

But, as I said, you can keep using Twisted as framework while using the asyncio functionality. This means you can use asyncio keywords, design patterns etc. while not throwing anything made so far out the window.

https://twistedmatrix.com/documents/current/core/howto/defer-intro.html#coroutines-with-async-await

I made the Deluge Streaming plugin and have designed a bunch around Twisted Web to serve files ( https://github.com/JohnDoee/thomas/blob/develop/thomas/outputs/http.py ) - The plugin itself also rely on Twisted and would require the same type of rewrite as Deluge.

Alternatively, I can just include Twisted in my plugin but I'd prefer not to have to do that.

comment:8 Changed 5 months ago by andar

I think that maybe running Twisted on top of asyncio might make sense for your plugin then.

https://twistedmatrix.com/documents/current/api/twisted.internet.asyncioreactor.html

import asyncio
from twisted.internet import asyncioreactor
asyncioreactor.install(asyncio.get_event_loop())

Maybe Deluge could do this for plugins so that we don't need to rewrite them?

I understand that we could wrap all the coroutines in Deferreds and keep using Twisted, but I really don't want to go down that route and would much rather remove Twisted and never deal with another Deferred again.

comment:9 Changed 4 months ago by andar

So I've made some progress, but I think that this was probably short-sighted. I've had to rewrite the component module and realized how this will further contemplate plugins. I think that if we were to move to asyncio, we should probably do it for all of Deluge and not just the core, it just really isn't feasible doing it piecemeal. The added cost of breaking all current plugins is probably too high as well.

I also forgot about how much a mess the code base is in places and we should probably just work to clean it up using Twisted before we even consider a more invasive change.

I've uploaded my current set of changes to https://github.com/aresch/deluge/tree/asyncio and I have a bunch more uncommitted changes that I could upload if anyone is interested. I think that I will abandon the work for the time being and maybe focus on a bit more on cleaning up the code where I can. Maybe we can repurpose some of the changes in my asyncio branch? Particularly, the protocol change and perhaps the new component module.

comment:10 Changed 4 months ago by andar

One important thing I forgot to mention and one of the main reasons I don't think it's wise to proceed is that I'm not sure how to accomplish standalone mode without moving the gtkui to asyncio too. It looks like it might get pretty complicated with how it functions.

Note: See TracTickets for help on using tickets.