#11817 Add twisted.internet.defer.race

[exarkun]

Scope and purpose

Fixes #11817

One thing I'm not sure about is whether race should accept and return Deferred or Awaitable (or something more esoteric .. Coroutine[Deferred[T], Any, T]?). The gymnastics to make cancellation work if it operates on Awaitable are not clear to me, though (and might just amount to "this works for any Awaitable as long as it is Deferred").

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

• The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
• A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
• The automated tests were updated.
• Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
  Our bot will trigger the review process, by applying the pending review label
  and requesting a review from the Twisted dev team.

[exarkun]

please review

[glyph]

I started reviewing this, and while I was picking my way through the very extensive comments in race, it struck me that this was a partial reimplementation of most of the logic in DeferredList. I don't particularly love DeferredList's implementation, but as the story goes, "one big pile is better than two little piles, and rather than bring that one up we decided to throw ours down".

I put a draft PR up here to see how you feel about that approach: #11819

[glyph]

I seem to have become mildly obsessed with golfing this problem. The other thing that occurred to me was that this generalizes quite neatly into something I find myself wanting a lot; specifically, the word "race" makes me think that you're getting 1st through Nth place rather than a single winner. So here's the version expressed in terms of that abstraction: https://github.com/twisted/twisted/pull/11820/files#diff-8d95429e1233c1c5f25e6f0acd93c6819745e8b6a6041b11b621c393cbaeac69R1514-R1538

[exarkun]

I started reviewing this, and while I was picking my way through the very extensive comments in race, it struck me that this was a partial reimplementation of most of the logic in DeferredList. I don't particularly love DeferredList's implementation, but as the story goes, "one big pile is better than two little piles, and rather than bring that one up we decided to throw ours down".

I put a draft PR up here to see how you feel about that approach: #11819

Can I convince you that instead of making the DeferredList pile bigger, we can chip away at it until it disappears? Nobody likes DeferredList. Nobody can remember how all of its corners work. Any change to any part of it involves understanding and preserving all of the other parts. Replacing it with 4 or 5 narrowly focused APIs (maybe with their own implementation, maybe with some shared implementation, if there's a way to do it that's simple enough) seems like a good idea to me.

[itamarst]

To second Jean-Paul: I used DeferredList recently and ended up with situation where it sometimes returned a list of tuples and sometimes returned a single tuple. It is irretrievably broken as an API and replacing it would be much better.

[glyph]

To second Jean-Paul: I used DeferredList recently and ended up with situation where it sometimes returned a list of tuples and sometimes returned a single tuple. It is irretrievably broken as an API and replacing it would be much better.

What a coincidence! In my attempted fix I updated the type annotation to correctly express this unfortunate brokenness :-\ so perhaps we should adopt that modification from that branch, if nothing else.

[glyph]

Can I convince you that instead of making the DeferredList pile bigger, we can chip away at it until it disappears? Nobody likes DeferredList. Nobody can remember how all of its corners work. Any change to any part of it involves understanding and preserving all of the other parts. Replacing it with 4 or 5 narrowly focused APIs (maybe with their own implementation, maybe with some shared implementation, if there's a way to do it that's simple enough) seems like a good idea to me.

I could get on board with this as a plan, but the implementation for race strikes me as the wrong end to address this. Specifically, implementing abstractions like "give me all these deferreds in the order that they fire" or "give me the first result but don't consume results or cancel everything else" would be an awkward and duplicative mess, adding tons of unnecessarily deep callbacks. In other words, I don't feel like having a one-off implementation of race gets us any closer to getting rid of DeferredList.

I like where I ended up with FiringOrder https://github.com/twisted/twisted/pull/11820/files#diff-8d95429e1233c1c5f25e6f0acd93c6819745e8b6a6041b11b621c393cbaeac69R1521-R1543 and it passes all the tests in this branch, with a 12 LOC implementation of race that requires far less explication, and a much more general-purpose way to deal with groups of Deferreds that you want to act on as a group. (It would require some light adjustment for the "don't consume results" case, but its general structure is amenable to such modification.)

So, let's forget about the DeferredList version, modulo the annotation fixes. How do you feel about the other one? (The DeferredList changes in that branch were just carried over, I can get rid of them.)

[glyph]

OK, I should not be holding this up because I think there's a better underlying implementation. The interface is fine; I can land a refactoring later.

I would consider renaming MultiFailure to FailureGroup, to parallel the stdlib's ExceptionGroup. I think we may want to support PEP 654 here at some point.

[exarkun]

OK, I should not be holding this up because I think there's a better underlying implementation. The interface is fine; I can land a refactoring later.

Thanks. And I have no problem with future factoring improvements to the implementation of this, I just don't have time to pursue them at the moment.

I would consider renaming MultiFailure to FailureGroup, to parallel the stdlib's ExceptionGroup. I think we may want to support PEP 654 here at some point.

👍