INCOMPATIBLE CHANGE: Towards the adoption of coroutines
Hello, I've just written up an idea for an approach to getting many parts of Twisted and Twisted-based applications onto coroutines (ie `async def`). Details are given on https://twistedmatrix.com/trac/ticket/10327 The change involves the potential for some incompatibilities - though as I described on the ticket, I think these are likely to be very rare in practice and worth imposing for the resulting payoff. This thread represents the start of the process for making incompatible changes to Twisted as described at https://twistedmatrix.com/documents/current/core/development/policy/compatib... There is not yet a branch but there is a patch in a comment on the issue (the change is extremely small) for anyone to try out or comment on. I will follow up when there is a version closer to what would be merged into trunk. Meanwhile, feedback is welcome. Jean-Paul
On 7/4/22 11:21, Jean-Paul Calderone wrote:
Hello,
I've just written up an idea for an approach to getting many parts of Twisted and Twisted-based applications onto coroutines (ie `async def`).
[quietly, from the house across the street] woo! yeah! right on! ... In terms of returning a coroutine, I believe that Python (especially under ASYNCIODEBUG=1) gets annoyed and spews warnings if a coroutine is not awaited (I believe when it is GC'd). Because returning a coroutine without scheduling it is possibly able to run into this case (e.g. a coroutine that performs a 'send clean goodbye' that is not scheduled until the connection is started to be shut down, but is never called on an unclean shutdown, just as a strawman), I think that it is completely valid to ignore it. Especially since returning a function-that-returns-a-couroutine that you then call when you need it is 1000x better of an API. That is to say, from my increasingly rusty memory of how all of this works, I believe this is a good way to go. - Amber
On 07/04/2022 02:21, Jean-Paul Calderone wrote:
Hello,
I've just written up an idea for an approach to getting many parts of Twisted and Twisted-based applications onto coroutines (ie `async def`).
Details are given on https://twistedmatrix.com/trac/ticket/10327 <https://twistedmatrix.com/trac/ticket/10327>
This looks great! Chris
Thanks Jean-Paul for the email On Thu, 7 Apr 2022 at 02:22, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:
There is not yet a branch but there is a patch in a comment on the issue (the change is extremely small) for anyone to try out or comment on. I will follow up when there is a version closer to what would be merged into trunk. Meanwhile, feedback is welcome.
Jean-Paul
I am not using coroutines. And I try to avoid using maybeDeferred But I think it is best to add support for coroutines in maybeDeferred. We have limited dev resources so it's OK to go with this backward incompatible change. -- Adi Roiban
On Wed, Apr 6, 2022 at 9:21 PM Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
Hello,
I've just written up an idea for an approach to getting many parts of Twisted and Twisted-based applications onto coroutines (ie `async def`).
Details are given on https://twistedmatrix.com/trac/ticket/10327
The change involves the potential for some incompatibilities - though as I described on the ticket, I think these are likely to be very rare in practice and worth imposing for the resulting payoff.
This thread represents the start of the process for making incompatible changes to Twisted as described at https://twistedmatrix.com/documents/current/core/development/policy/compatib...
Thanks to everyone who supplied feedback. Since it seemed positive, I've taken the next step in the process - https://github.com/twisted/twisted/pull/1718 is a PR that implements the proposed change in behavior. According to the compatibility policy, this should be accepted if three committers approve and there are no objections for one week. I'm not in any huge rush so I don't imagine trying to merge this in exactly 7 days from this email (in fact, I'm on vacation all next week...) Please have a look and let me know if you spot any problems. Thanks, Jean-Paul
There is not yet a branch but there is a patch in a comment on the issue (the change is extremely small) for anyone to try out or comment on. I will follow up when there is a version closer to what would be merged into trunk. Meanwhile, feedback is welcome.
Jean-Paul
On Apr 11, 2022, at 11:29 AM, Jean-Paul Calderone <calderone.jeanpaul@gmail.com> wrote:
On Wed, Apr 6, 2022 at 9:21 PM Jean-Paul Calderone <exarkun@twistedmatrix.com <mailto:exarkun@twistedmatrix.com>> wrote: Hello,
I've just written up an idea for an approach to getting many parts of Twisted and Twisted-based applications onto coroutines (ie `async def`).
Details are given on https://twistedmatrix.com/trac/ticket/10327 <https://twistedmatrix.com/trac/ticket/10327>
The change involves the potential for some incompatibilities - though as I described on the ticket, I think these are likely to be very rare in practice and worth imposing for the resulting payoff.
This thread represents the start of the process for making incompatible changes to Twisted as described at https://twistedmatrix.com/documents/current/core/development/policy/compatib... <https://twistedmatrix.com/documents/current/core/development/policy/compatib...>
Thanks to everyone who supplied feedback. Since it seemed positive, I've taken the next step in the process - https://github.com/twisted/twisted/pull/1718 <https://github.com/twisted/twisted/pull/1718> is a PR that implements the proposed change in behavior.
According to the compatibility policy, this should be accepted if three committers approve and there are no objections for one week.
I'm not in any huge rush so I don't imagine trying to merge this in exactly 7 days from this email (in fact, I'm on vacation all next week...)
Please have a look and let me know if you spot any problems.
It’s not a huge problem, but I reviewed on github and my only real feedback is “I think we should use a regular `isinstance`.” But I’ll leave it to you as the author of this change to make the call. Hopefully another committer or two can have a look soon. -g
On Mon, Apr 11, 2022 at 6:31 PM Glyph <glyph@twistedmatrix.com> wrote:
On Apr 11, 2022, at 11:29 AM, Jean-Paul Calderone < calderone.jeanpaul@gmail.com> wrote:
On Wed, Apr 6, 2022 at 9:21 PM Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
Hello,
I've just written up an idea for an approach to getting many parts of Twisted and Twisted-based applications onto coroutines (ie `async def`).
Details are given on https://twistedmatrix.com/trac/ticket/10327
The change involves the potential for some incompatibilities - though as I described on the ticket, I think these are likely to be very rare in practice and worth imposing for the resulting payoff.
This thread represents the start of the process for making incompatible changes to Twisted as described at https://twistedmatrix.com/documents/current/core/development/policy/compatib...
Thanks to everyone who supplied feedback. Since it seemed positive, I've taken the next step in the process - https://github.com/twisted/twisted/pull/1718 is a PR that implements the proposed change in behavior.
According to the compatibility policy, this should be accepted if three committers approve and there are no objections for one week.
I'm not in any huge rush so I don't imagine trying to merge this in exactly 7 days from this email (in fact, I'm on vacation all next week...)
Please have a look and let me know if you spot any problems.
It’s not a huge problem, but I reviewed on github and my only real feedback is “I think we should use a regular `isinstance`.” But I’ll leave it to you as the author of this change to make the call.
Hopefully another committer or two can have a look soon.
Thank you to the three reviewers so far - glyph, twm, and adiroiban. I believe that the PR is now in its final form and has been approved by three Twisted committers. So at this point, the change will land. As I mentioned previously, I probably won't land it Monday but I may towards the end of the week - unless there is further feedback before then. Jean-Paul
participants (6)
-
Adi Roiban -
Amber Brown (hawkowl) -
Chris Withers -
Glyph -
Jean-Paul Calderone -
Jean-Paul Calderone