Add to todo list: give the tests meaningful names.

Yury's version also has a convenience method that returns the set of sub-exception types.

That's probably useful only for tests. We could moveassertExceptionIsLike to a mixin in test.support.

The BaseException hacks are clever but awkward to replicate in a subclass.

Subclassing was an afterthought. We assumed it's not needed, now I think we're assuming it would be rare. If you do subclass here, make sure you definederive(). Otherwise split() will not create parts of your type.

The str() of an EG is rather sparse (Yury's version adds the number of sub-exceptions and their types).

We could, it might be too long then though.

Regarding__cancel_requested__, I support the change and have a follow-up proposal: returnFalse fromcancel() call if__cancel_requested__ was setand don't override the cancellationmsg in this case. I believe it makesTask.cancel() design in line withFuture.cancel().

If we have an agreement for it, I can prepare a pull request on the weekend.

Regarding__cancel_requested__, I support the change and have a follow-up proposal: returnFalse fromcancel() call if__cancel_requested__ was setand don't override the cancellationmsg in this case. I believe it makesTask.cancel() design in line withFuture.cancel().

So it would, but I am worried about breaking code that depends on the current behavior -- often developers have no idea how to debug such a failure, and it would give 3.11 a bad rep "randomly breaks async apps".

But maybe it's not so bad? Would any tests break?

If we have an agreement for it, I can prepare a pull request on the weekend.

Give it a whir and we can see if it's viable. If not, we can do something less drastic to just expose the cancel-requested state (maybe that's justt.cancelled()) and add an API to reset it.

Add to todo list: give the tests meaningful names.

Done (i.e., added to the todo list :-).

Yury's version also has a convenience method that returns the set of sub-exception types.

That's probably useful only for tests. We could moveassertExceptionIsLike to a mixin in test.support.

Perhaps. We'll see how often this comes up. I find my current solution manageable, even though it isn't as concise as Yury's code.

The BaseException hacks are clever but awkward to replicate in a subclass.

Subclassing was an afterthought. We assumed it's not needed, now I think we're assuming it would be rare. If you do subclass here, make sure you definederive(). Otherwise split() will not create parts of your type.

So we have a real decision to make here: do we needTaskGroupException or can we just raiseExceptionGroup?

The str() of an EG is rather sparse (Yury's version adds the number of sub-exceptions and their types).

We could, it might be too long then though.

Yeah, we might have to truncate if there are too many. But I already know that I've been confused regularly when I didprint(exc) whereexc was an EG, and it printedjust the message. (repr(exc) is much better.) Maybe just put the count in there? Presumably this would be a change in the PEP based on practical experience during the alpha/beta period.

Figure out why intest_taskgroup_14 I get a nestedExceptionGroup, while the original got a flatMultiError.

Mystery solved:test_taskgroup_14 doesnot get a flatMultiError! It's just thatassertRaisesRegex(E, pat) looks forpat anywhere in the exception's error message, andMultiError includes the complete traceback for all suberrorsin the message! (Because it has to work with Python versions that don't print nested tracebacks.)

I wrote a small test program and found that the full error message contains the nestedMultiError:

edb.common.taskgroup.TaskGroupError: unhandled errors in a TaskGroup; 1 sub errors: (TaskGroupError) + TaskGroupError: unhandled errors in a TaskGroup; 1 sub errors: (ZeroDivisionError) + ZeroDivisionError: division by zero |   File "t.py", line 7, in crash_after |     1/0 |   File "t.py", line 13, in amain |     h.create_task(crash_after(0.5)) |   File "/Users/guido/edgedb/edb/common/taskgroup.py", line 170, in __aexit__ |     raise me from None

So theassertRaisesRegex() call matched on the second line of that. (@1st1: It's not obvious from the way the test was written that this was intentional.)

FWIW I am strongly leaning towards not having a customTaskGroupError. Theasyncio package only defines a few custom exceptions, and those seem semantically important (e.g. they convey that a very specific error occurred). It usually raises builtin exceptions likeTypeError,ValueError,RuntimeError orConnectionResetError.

UPDATE: Done.

I looked more into handling ofBaseException.

This is the core of test 20:

asyncdefcrash_soon():awaitasyncio.sleep(0.1)1/0asyncdefnested():try:awaitasyncio.sleep(10)finally:raiseKeyboardInterruptasyncdefrunner():asyncwithtaskgroups.TaskGroup()asg:g.create_task(crash_soon())awaitnested()

We expect therunner() to terminate withKeyboardInterrupt. This works, since theKeyboardInterrupt transfers control directly into__aexit__, which squirrels it away and eventually raises it (without using an exception group).

Test 21 is almost the same except thecrash_soon() coroutine raisesKeyboardInterrupt andnested() raises a regular exception. We would still likerunner() to exit withKeyboardInterrupt, but in this case theKeyboardInterrupt exception has to travel through asyncio, which (intentionally) treatsKeyboardInterrupt andSystemExit special so that they immediately terminate the event loop. Therefore, test 21 doesn't work. (To make it work, asyncio would have to stop makingKeyboardInterrupt andSystemExit special cases.)

I did add new tests that parallel test 20 and 21 but useMyBaseExc instead ofKeyboardInterrupt, and those raise, as expected, aBaseExceptionGroup wrapping both theMyBaseExc and the other exception.

I think there are good enough reasons why asyncio has these special cases forKeyboardInterrupt andSystemExit, so for now I'll leave test 21 disabled.

Design question: Should we allow creating new tasks while__aexit__() is already waiting?

I just chatted with some Trio folks and they had decent use cases for allowing this. I'm going to look into how easy it would be to add this functionality.@1st1

Yeah, we might have to truncate if there are too many. But I already know that I've been confused regularly when I didprint(exc) whereexc was an EG, and it printedjust the message. (repr(exc) is much better.) Maybe just put the count in there? Presumably this would be a change in the PEP based on practical experience during the alpha/beta period.

I madebpo46729 for this and adraft PR so we can see the impact on tracebacks in the tests.

I just chatted with some Trio folks and they had decent use cases for allowing this. I'm going to look into how easy it would be to add this functionality.@1st1

Looked pretty easy, so I made this change.

(However, as soon as cancel scopes came up they started saying asyncio was dumb so I gave up asking more about that.)

I didn't say asyncio was dumb. I said the way cancellation works is. You noted this yourselfhere. My generalization was unjust and I apologize for that.

To get back to the matter at hand: localized timeouts are a pattern I often see in the wild, and they're somewhat problematic without cancel scopes. Take this example:

asyncwithtimeout(5):   ...

Such a construct is provided not only by AnyIO and trio, butasyncio_timeout as well. The problem arises when the task is cancelled as a whole and the timeout expires, both before the event loop has a chance to raise the cancellation exception in the task. How, then, do you know if you need to simply exit thetimeout() context manager or the entire task?

@agronholm If you want to discuss cancel scopes please open a new bpo issue.

🤖 New build scheduled with the buildbot fleet by@gvanrossum for commit9712241 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Agree. I personally don't see any blocker.
But I want to improve tests a little, sure.

Okay, let's merge once buildbots are green, then we can improve tests and docs in later PRs.

BTW, are you interested in having something like your async-timeout in the stdlib as well? This might satisfy the Trio folks' desire for cancel scopes. (In order to work properly it would have to use .uncancel().)

BTW, are you interested in having something like your async-timeout in the stdlib as well? This might satisfy the Trio folks' desire for cancel scopes. (In order to work properly it would have to use .uncancel().)

Yes, sure. I think the current API is stable and consistent. The only known problem is a case when timeout occurs for a task that was cancelled on the previous loop iteration.
Now, this bug can be fixed easily because of thetask.cancelling() method and changed.cancel() behavior. I can prepare a pull request fortimeout async context manager inclusion into asyncio.

upd the mentionedasync-timeout bug is actually auto-fixed after this PR merging, no timeout source code change is required. The same probably is true forasyncio.wait_for().

BTW, are you interested in having something like your async-timeout in the stdlib as well? This might satisfy the Trio folks' desire for cancel scopes. (In order to work properly it would have to use .uncancel().)

Yes, sure.

Andrew, I also think it's time for us to add a context manager for timeouts to asyncio.

Are you folks acquainted withhttps://pypi.org/project/quattro/, since there's talk of timeout context managers in the stdlib?

It's also a little unfortunate Quattro task groups weren't used instead, they are identical but with type annotations (and I think I fixed a minor issue with something, pickling?)

A simple "thank you" would have sufficed.

@Tinche

It's also a little unfortunate Quattro task groups weren't used instead, they are identical but with type annotations (and I think I fixed a minor issue with something, pickling?)

I've taken a quick look, the file seems identical with what Guido committed (except typing). If there's something you fixed and you can remember what it was, please file a bug or create a PR.

Nit: I've also noticed that you removed the original copyright comment from the file. Not that I care in this specific case, but you shouldn't remove copyright notices from files you borrow from other projects unless you're explicitly permitted to do so. (Thanks for crediting in README though!)

It's also a little unfortunate Quattro task groups weren't used instead, they are identical but with type annotations

Type annotations are still not a thing in the standard library. Were you hoping for the code to be type annotated?

@Tinche On another note, I am currently writing a BPO for potential low level asyncio cancellation machinery updates to support the implementation of cancel scopes and level cancellation in asyncio. The writing process has largely turned into a rubber ducking session and led to new insights on my part. The interesting aspect of Quattro is that it implements cancel scopes along with task groups, which this PR does not. It would be interesting to compare our implementations privately. I'll reach out to you by Gitter so as not to unnecessarily flood this PR.

Nit: I've also noticed that you removed the original copyright comment from the file.

Sorry, I restored it just now. To me it's just clutter since the repo already has a license included. I see the TaskGroup here doesn't have it, so I might switch to that then ;)

@Tinche I've created a separate issue for cancel scopes:bpo-46771.

Sorry, I restored it just now. To me it's just clutter since the repo already has a license included. I see the TaskGroup here doesn't have it, so I might switch to that then ;)

You should include the full copyright comment.

HOWEVER, this introduces a change in behavior around task cancellation:

• A task that catchesCancelledError is allowed to run undisturbed (ignoring further.cancel() callsand allowing any number ofawait calls!) until it either exits or calls.uncancel().

While this change was merged, it is still being debated, and we may revise this. See e.g.bpo-46771. (In general, what was merged is intended as a starting point for further experiments and discussion, at least until feature freeze / beta 1, late May.)