-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
gh-108951: add TaskGroup.stop() #127214
base: main
Are you sure you want to change the base?
gh-108951: add TaskGroup.stop() #127214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is not a full review, just a couple of questions.
@@ -414,53 +433,6 @@ reported by :meth:`asyncio.Task.cancelling`. | |||
Improved handling of simultaneous internal and external cancellations | |||
and correct preservation of cancellation counts. | |||
|
|||
Terminating a Task Group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docs make sense for older versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably recommending a backport module on PyPI would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These docs were just added in September, and backported to 3.13 and 3.12.
It's my understanding that the deletion here wouldn't affect the docs of previous versions.
As for this PR, I'd expected it to be backported as far back as is allowed by policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@belm0 are you interested in applying this change and any previous changes to my taskgroup backport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new API, so we won't backport it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about backporting to pypi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sure. PyPI is off limits :)
@@ -997,6 +999,69 @@ class MyKeyboardInterrupt(KeyboardInterrupt): | |||
self.assertIsNotNone(exc) | |||
self.assertListEqual(gc.get_referrers(exc), no_other_refs()) | |||
|
|||
async def test_taskgroup_stop_children(self): | |||
async with asyncio.TaskGroup() as tg: | |||
tg.create_task(asyncio.sleep(math.inf)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these tasks should look like this?
async def task(results, num):
results.append(num)
await asyncio.sleep(math.inf)
results.append(-num)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can assert what was in results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this particular test, I chose a different test approach, which is to wrap in asyncio.timeout()
.
For the other tests using count,
I'm not sure it's much value, since the test code is only a few lines and there is only one possible path through it. So count
result of 0, 1, or 2 each have deterministic meaning that's obvious by looking at the code.
|
||
with self.assertRaises(ExceptionGroup): | ||
async with asyncio.TaskGroup() as tg: | ||
tg.create_task(raise_exc(tg)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if some tasks cancels itself? How would this interact with .stop()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the case where a child task calls stop()
on its parent TaskGroup, or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cancellations (and thus taskgroup stops) happen when the next await …
actually yields to the asyncio loop. Who the caller of the cancel or stop operation is doesn't matter.
Co-authored-by: sobolevn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why call it TaskGroup.stop()
and not TaskGroup.cancel()
? I'd be more in favor of the latter name.
Short-circuiting of task groups is a very common, useful, and normal, so make it a first-class operation.
Any evidence of this statement? I'd like you to write up technical motivation + examples. That will be useful for the docs.
And speaking of the documentation, you should also show some recipies of how this would be used. Like are you supposed to use this API from within the task group async with
clause? Or can you pass the task group to some remote task?
I haven't reviewed the actual implementation in detail yet.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
This doesn't work in the case that the body of the task group throws an exception, as in this code: async def test_taskgroup_throw_inside(self):
class MyError(RuntimeError):
pass
should_get_here = False
try:
async with asyncio.TaskGroup() as tg:
tg.create_task(asyncio.sleep(0.1))
tg.stop()
self.assertEqual(asyncio.current_task().cancelling(), 1)
raise MyError
self.fail() # <-- reaches here instead of raising ExceptionGroup([MyError()])
except* MyError:
self.assertEqual(asyncio.current_task().cancelling(), 0)
should_get_here = True
self.assertTrue(should_get_here) The problem is that the new code in the if et is not None and not issubclass(et, exceptions.CancelledError):
self._errors.append(exc) One option is move these lines earlier, before the I'd still suggest my original proposal (see the issue) where you just add a single line As a separate point, I'd suggest that the tests could do with a few more checks that |
I'd also prefer
In trio the equivalent is I have years experience developing a non-trivial, production async app, which I've presented at PyCon JP. Anecdotally, I can't imagine how painful and unproductive it would be to not have short circuiting of task groups.
All is on the table: stop from within the TaskGroup body, from a child, from some other entity you've passed the bound stop() method to. |
Well, that's exactly what it does, isn't it? The fact that the cancelled taskgroup catches the Also, trio and anyio already call this operation |
Short-circuiting of task groups is a very common, useful, and normal, so make it a first-class operation. The recommended approach to date-- creating a task just to raise an exception, and then catch and suppress the exception-- is inefficient, prone to races, and requires a lot of boilerplate.
taskgroup.stop
method #108951📚 Documentation preview 📚: https://cpython-previews--127214.org.readthedocs.build/