strict_exception_groups=True "leaks" the implementation-detail nursery in nursery.start(fn)

[Zac-HD]

Currently, exceptions raised from a nursery.start()'ed function will be wrapped in extra ExceptionGroup (aka MultiError), iff it's raised before task_status.started() is called.

I think that for consistency, we should pull such exceptions (which might themselves be ExceptionGroups) out of the group added by the starter-nursery.

import trio

async def startable_fn_which_doesnt_contain_a_nursery(task_status):
    if ...:
        print("Something went wrong")
        raise RuntimeError  # this might be wrapped in an ExceptionGroup
    task_status.started()
    raise ValueError  # ...but this will never be wrapped!

async def main():
    async with trio.open_nursery() as nursery:
        try:
            # Uh-oh!  With strict_exception_groups=True, the hidden inner nursery
            # in `nursery.start` wraps our RuntimeError in an ExceptionGroup :-/
            await nursery.start(startable_fn_which_doesnt_contain_a_nursery)
        except RuntimeError:
            print("No worries, we'll try something else.")

if __name__ == "__main__":
    trio.run(main, strict_exception_groups=False)  # passes
    print("\n==============================================================\n")
    trio.run(main, strict_exception_groups=True)  # fails!

Related to #2544 and possibly #1457.

[njsmith]

Oh, nice catch. Yeah, raising an EG there is surprising, and unnecessary – we know that that nursery only has one task inside it.

…

On Thu, Mar 16, 2023, 18:14 Zac Hatfield-Dodds ***@***.***> wrote: Currently, exceptions raised from a nursery.start()'ed function will be wrapped in *extra* ExceptionGroup (aka MultiError), *iff* it's raised before task_status.started() is called. I think that for consistency, we should pull such exceptions (which might themselves be ExceptionGroups) out of the group added by the starter-nursery. import trio async def startable_fn_which_doesnt_contain_a_nursery(task_status): if ...: print("Something went wrong") raise RuntimeError # this might be wrapped in an ExceptionGroup task_status.started() raise ValueError # ...but this will never be wrapped! async def main(): async with trio.open_nursery() as nursery: try: # Uh-oh! With strict_exception_groups=True, the hidden inner nursery # in `nursery.start` wraps our RuntimeError in an ExceptionGroup :-/ await nursery.start(startable_fn_which_doesnt_contain_a_nursery) except RuntimeError: print("No worries, we'll try something else.") if __name__ == "__main__": trio.run(main, strict_exception_groups=False) # passes print("\n==============================================================\n") trio.run(main, strict_exception_groups=True) # fails! — Reply to this email directly, view it on GitHub <#2611>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEU42C4PFFH4FMBF6BWODLW4O3HHANCNFSM6AAAAAAV55PKMU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Zac-HD]

we know that that nursery only has one task inside it.

Unfortunately, #1599:

In theory, there should only be one task in the old nursery, since the nursery isn't exposed anywhere outside the body of start(). In practice, it's possible to access the old nursery via current_task().parent_nursery in the newly-started task, so we may have some users who are starting more tasks there.

[njsmith]

OK true, and ideally we'd put a guard-rail there as discussed in #1600. But either way it's very much in the "if you break it you get to keep both pieces" zone; I don't think we should let that affect how `start` behaves under normal circumstances.

…

On Thu, Mar 16, 2023 at 9:27 PM Zac Hatfield-Dodds ***@***.***> wrote: we know that that nursery only has one task inside it. Unfortunately, #1599 <#1599>: In theory, there should only be one task in the old nursery, since the nursery isn't exposed anywhere outside the body of start(). In practice, it's possible to access the old nursery via current_task().parent_nursery in the newly-started task, so we may have some users who are starting more tasks there. — Reply to this email directly, view it on GitHub <#2611 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEU42EXU5JDGTSOCDYBAYTW4PR2VANCNFSM6AAAAAAV55PKMU> . You are receiving this because you commented.Message ID: ***@***.***>

-- Nathaniel J. Smith -- https://vorpus.org <http://vorpus.org>

[Zac-HD]

Yeah, I think "if we're using strict exception groups and the inner nursery raises an exception group with a single exception in it (subtree, not necessarily leaf!) unwrap it concat the tracebacks" is sufficient. We just can't assume that there's only ever one subtree of the top-level group.