| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you! This is not a full review, just a couple of questions.
Sorry, something went wrong.
|
|
||
| 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.
Maybe these tasks should look like this?
async def task(results, num):
results.append(num)
await asyncio.sleep(math.inf)
results.append(-num)
Sorry, something went wrong.
There was a problem hiding this comment.
So we can assert what was in results
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Co-authored-by: sobolevn <mail@sobolevn.me>
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
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 _aexit() method, if not self._errors: return True, is essentially duplicating the if self._errors test later in the function, but in between self._errors is changed by these two lines: if et is not None and not issubclass(et, exceptions.CancelledError):
self._errors.append(exc)One option is move these lines earlier, before the if self._parent_cancel_requested statement. Then both tests are checking the same thing. This seems to work. I'd still suggest my original proposal (see the issue) where you just add a single line return True to the very end of _exit() instead of these changes. This avoids duplicating the test in the first place and avoids changing the control flow and, personally, I find it easier to follow. As a separate point, I'd suggest that the tests could do with a few more checks that asyncio.current_task().cancelling() is correct, like the ones in the test above in this comment. |
Sorry, something went wrong.
I'd also prefer cancel(), but per Guido it would be confusing since such a method would be expected to raise CancelledError, and he suggested stop().
In trio the equivalent is nursery.cancel_scope.cancel(), which has > 1,000 hits on github, despite the unpopularity of trio. 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. |
Sorry, something went wrong.
Well, that's exactly what it does, isn't it? The fact that the cancelled taskgroup catches the CancelledErrors raised by itself doesn't change that. You don't get to wait on taskgroups the way you wait on tasks, thus the exception isn't visible like when you await on a cancelled task, but that's a minor detail IMHO. Also, trio and anyio already call this operation cancel. |
Sorry, something went wrong.
|
|
||
| Ways to use :meth:`cancel`: | ||
|
|
||
| * call it from the task group body based on some condition or event |
There was a problem hiding this comment.
Probably you want code examples for all of these?
Sorry, something went wrong.
|
@belm0 -- I take it you are not planning to add cancel messages and you are not planning to add more examples to the docs? Once you confirm that I will approve and merge. |
Sorry, something went wrong.
There was a problem hiding this comment.
I take it you are not planning to add cancel messages and you are not planning to add more examples to the docs?
I've been working through the PR's items in some priority order and hadn't reached those (you've caught me in the middle of quite a hectic week).
I've just committed a few tweaks to the added docs, but no cancel examples. Task groups are quite powerful and I've seen other async libs build up examples over several pages. The existing docs are lacking here so I don't have the foundation to add the cancel() examples. Expanded TaskGroup docs need to be written by someone who lives and breathes asyncio daily and is intimate with the library's nuances (that was once me for trio, but not asyncio).
For cancel(msg) I've made another comment on that thread-- not being bought into the idea I'm reluctant to take on the extra impl/doc/testing in this PR.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM; I'll merge it.
I do have some nits on the docs still but they don't need to hold up the merge.
Sorry, something went wrong.
|
@belm0 Thanks for your perseverance! This looks like a useful addition. Additionally we probably need a "what's new" entry, and I have some nits on the docs (mostly that I find the term "short-circuit" confusing, I've not seen it in the context of asyncio or task groups). |
Sorry, something went wrong.
|
I was unaware of this effort. Good to see this being added. I have some nits on the docs, though. It says:
As opposed to what? CancelledError being raised from the taskgroup? Does cancel() have any effect on that? And the news item:
Surely it will exit with errors if the tasks raise any non-cancellation exceptions after TaskGroup.cancel()? |
Sorry, something went wrong.
I think that's supposed to be "as opposed to cancelling the task in which the taskgroup is running". |
Sorry, something went wrong.
But it does cancel the host task too? It just uncancels it when exiting the task group. |
Sorry, something went wrong.
That would be … quite non-productive. |
Sorry, something went wrong.
|
AnyIO task groups and Trio nurseries do that too. Do you find that unproductive? |
Sorry, something went wrong.
|
@agronholm Are you still talking about the docs or are you now disagreeing with how it works? If the former, feel free to send a PR. If the latter, I suggest reading the full discussion in the issue first, and then everything here. Use an AI to summary if you want to. |
Sorry, something went wrong.
I'm not trying to argue about the functionality, I only have some slight problems about the wordings and how they could be misleading. |
Sorry, something went wrong.
|
@agronholm Misunderstanding here. "Cancel the host task", in asyncio parlance, meant cancelling the task directly, not the taskgroup. in that case the taskgroup won't eat the cancellation exception. |
Sorry, something went wrong.
There is no way to accomplish this on asyncio without calling Task.cancel(). And yes, the task group will eat the cancellation exception (via uncancellation) unless .cancel() was also called from elsewhere. |
Sorry, something went wrong.
Fixes python#108951 Co-authored-by: sobolevn <mail@sobolevn.me> Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com> Co-authored-by: Guido van Rossum <guido@python.org>
|
whatsnew entry and minimal doc tweaks in #149031 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.
📚 Documentation preview 📚: https://cpython-previews--127214.org.readthedocs.build/