Nurseries cannot propagate StopAsyncIteration exceptions #393

Activity

1. njsmith commented on Jan 5, 2018

   

   njsmith

   on Jan 5, 2018

   Last edited by njsmith

   Member

   More actions

   Oh wow, this is a tricky bug you found. At first I thought it was #229, but it's actually different. (BTW, that concurrent zip is really cool! Even if it does seem to be uniquely suited to tickling this very weird bug :-).) Anyway, here's what's happening:

   A popular way to implement context managers is by writing a generator, and then decorating it with contextlib.contextmanager. This same pattern can be used to write async context managers, by writing an async generator and then using an appropriately-implemented async context manager decorator. Trio has an internal implementation of this decorator (trio._util.acontextmanager), and that's how trio.open_nursery is implemented. So open_nursery looks roughly like:

   @acontextmanager
   async def open_nursery():
       # ... do setup stuff
       try:
           yield nursery
       except BaseException as exc:
           nursery._record_exception(exc)
       await nursery._wait_for_all_tasks_to_exit()
       raise nursery._combine_exceptions()

   Here, exactly one of the background tasks raised StopAsyncIteration, so the last line ends up being raise that_stop_async_iteration_object.

   However, there's a problem: you're not allowed to raise StopAsyncIteration inside an async generator! According to PEP 479, if you try, then it gets replaced by a RuntimeError. This is actually a good thing, because the way generators work, a regular return is converted into a StopAsyncIteration (just like falling off the end of a generator function causes StopIteration to be raised), so if it wasn't converted into a RuntimeError then it would have ended up getting swallowed by the decorator and mis-interpreted as a bare return, which would have been even more confusing. But... either way, this isn't going to work.

   Our @acontextmanager decorator has some special code to detect this problem when the exception was actually raised inside the body of the async with block, and fix things up again – which means that code like this works the way you'd expect:

   async with trio.open_nursery() as nursery:
       raise StopAsyncIteration

   But the only reason this works is because the @acontextmanager catches the StopAsyncIteration first before throwing it into the open_nursery code, so it can recognize the special RuntimeError when it comes back out. In your case, the StopAsyncIteration is being raised by a background task and gets routed into the nursery via the nursery machinery, so @acontextmanager doesn't have any way to recognize it, and we can't use this special trick.

   So this really depends on a bunch of arcane details of how async context managers, async generators, and the nursery implementation all interact. Neat!

   But, uh... what do we do about it?

   I think the only option is to give up on using @acontextmanager to implement open_nursery, and switch to implementing the async context manager protocol directly. This is a bit tricky, but I've actually done most of the work already (see also #340). I guess this will be the thing that pushes us over into actually switching to that! (And this will also fix #229, so that's nice.)

   While you're waiting, though, here's a workaround: the problem is specifically with having a function called by nursery.start_soon that raises StopAsyncIteration. If you handle the exception inside the function, instead of letting it escape, then that should work. So something like (untested):

   class async_zip(object):
   
       def __init__(self, *largs):
           # I added the __aiter__() call here; it's nothing to do with this bug, but you should have it :-)
           self.nexts = [obj.__aiter__().__anext__ for obj in largs]
           # We put the "are we done?" state on the object so _accumulate can see it
           self.done = False
   
       async def _accumulate(self, f, items, i):
           try:
               items[i] = await f()
           except StopAsyncIteration:
               self.done = True
   
       def __aiter__(self):
           return self
   
       async def __anext__(self):
           if self.done:
               # catch nasty users who try to re-iterate us after we said we were finished
               # (another advantage of having it on the object)
               raise StopAsyncIteration
   
           nexts = self.nexts
           items = [None, ] * len(nexts)
   
           async with trio.open_nursery() as nursery:
               for i, f in enumerate(nexts):
                   nursery.start_soon(self._accumulate, f, items, i)
   
           if self.done:
               raise StopAsyncIteration
           # I also added the call to tuple() here because that's how zip() is supposed to work :-)
           return tuple(items)

   Alternatively you could implement it as an async generator yourself (this is using the native async generator syntax, so requires Python 3.6+). Maybe this is clearer? I dunno:

   async def async_zip(*aiterables):
       aiterators = [aiterable.__aiter__() for aiterable in aiterables]
       done = False
       while True:
           items = [None] * len(aiterators)
   
           async def fill_in(i):
               try:
                   items[i] = await aiterators[i].__anext__()
               except StopAsyncIteration:
                   nonlocal done
                   done = True
   
           async with trio.open_nursery() as nursery:
               for i in range(len(aiterators):
                   nursery.start_soon(fill_in, i)
   
           if done:
               break
           yield tuple(items)
2. njsmith

   changed the title [-]Issue with async iterators[/-] [+]Nurseries cannot propagate StopAsyncIteration exceptions[/+] on Jan 5, 2018
3. imrn commented on Jan 5, 2018

   

   imrn

   on Jan 5, 2018 via email

   More actions

   acontextmanager is also a missing piece of python stdlib. Are you considering to upstream it?
4. njsmith commented on Jan 5, 2018

   

   njsmith

   on Jan 5, 2018 via email

   Member

   More actions

   It's already upstream, as contextlib.asynccontextmanager. (I had nothing to do with this :-).) The problem is that this won't actually ship until Python 3.7 comes out, which is ~6 months away, and longer than that to become the minimum supported version. I'll probably move trio's version into async_generator for those who need it sooner. (If someone wants to make a PR for this then please do!)

   …

   On Jan 5, 2018 7:42 AM, "Imran Geriskovan" ***@***.***> wrote: acontextmanager is also a missing piece of python stdlib. Are you considering to upstream it? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOaBrHiOscMgHjOW1HVNP64-NuVxY3ks5tHkLKgaJpZM4RUADX> .
5. matham commented on Jan 5, 2018

   

   matham

   on Jan 5, 2018

   ContributorAuthor

   More actions

   Ahh, thanks for explaining that (and little code fixes). Your first work-around will work nicely (I'm still on 3.5.2).
6. njsmith

   closed this as completedin #612on Aug 22, 2018