Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider to convert inlineCallbacks to async/await #7371

Open
pmisik opened this issue Jan 5, 2024 · 2 comments
Open

Consider to convert inlineCallbacks to async/await #7371

pmisik opened this issue Jan 5, 2024 · 2 comments
Labels

Comments

@pmisik
Copy link
Contributor

pmisik commented Jan 5, 2024

Twisted documentation for Inline callbacks - using ‘yield’ states

Unless your code supports Python 2 (and therefore needs compatibility with older versions of Twisted), writing coroutines with the functionality described in “Coroutines with async/await” is preferred over inlineCallbacks. Coroutines are supported by dedicated Python syntax, are compatible with asyncio, and provide higher performance.

As Buildbot dropped support for Python2 there is opportunity to convert inlineCallbacks to async/await.

I’m not sure if it helps.

Some thoughts on why and how to do this are given in matrix-org/synapse#7988 (comment)

This is related blogpost https://patrick.cloke.us/posts/2021/06/11/converting-twisteds-inlinecallbacks-to-async/

@p12tic
Copy link
Member

p12tic commented Jan 25, 2024

Buildbot will transition to async/await at some point. One problem is that moving from inlineCallbacks to async/await changes return type of a function from defer.Deferred to coroutine. Users may be relying on additional functionality exposed by defer.Deferred() class. Thus the migration will be rather complex.

@p12tic p12tic added the feature label Jan 25, 2024
@tdesveaux
Copy link
Contributor

This would help a lot with type hints as inlineCallback function return types are not really straightforward.
They need to be typed with Generator[Deferred[Any], Any, _ReturnType] (maybe Twisted provide a better type? I could not find it though).
There could be a type util added to make it a bit more palatable, something like:

from typing import Any
from typing import Generator
from typing import TypeVar

T = TypeVar('T')
InlineCallBackType = Generator[defer.Deferred[Any], Any, T]

...

@defer.inlineCallbacks
def _function() -> InlineCallbackType[int]:
    yield ...
    ...
    return 1

but it's a bit annoying when a subclass override a method with an implementation that does not need yielding.
It leads to code that is only there to placate type check (but maybe someone that knows more about twisted has a better way?)

@defer.inlineCallbacks
def _function() -> InlineCallbackType[int]:
    yield defer.succeed(None)
    return 1

Looking a bit at the matrix code pre-async transition, it looks like they didn't bother with typing the returns of their inlineCallback function.

Buildbot will transition to async/await at some point. One problem is that moving from inlineCallbacks to async/await changes return type of a function from defer.Deferred to coroutine. Users may be relying on additional functionality exposed by defer.Deferred() class. Thus the migration will be rather complex.

Looks like that can be handled pretty easily by user code with Deferred.fromCoroutine (doc).
But it would make things awkward for plugins that want to support multiple versions, but should be ok to make the transition in the next major version?
There is an helper for this mentioned in the matrix issue which is part of twisted (ensureDeferred):

There are remaining uses of ensureDeferred, which has the unfortunate side-effect of converting async to inlineCallbacks.

That could allow a more aggressive transition to take place in the next minor version.

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

No branches or pull requests

3 participants