Unhelpful error message when calling an uncallable object in an async context

[Badg]

I've seen #65, but I'm not sure if this is the same. Happy to close if so!

So tl;dr here is that I made a silly mistake, and inside trio my silly mistake got converted into a TrioInternalError that swallowed the original problem. Might there be an opportunity for a raise TrioInternalError() from exc or something? It would be a great quality of life improvement for... what is admittedly a pretty unusual edge case!

---

A story: while doing a refactor recently, I found myself in a rather confused headspace, and managed to forget the spelling of __aenter__, foolishly thinking that I could do something like this:

class Foo:
    async def __aenter__(self, some_var):
        ...

...

async with foo(bar):
   ...

Which is, of course, wrong (in case somebody stumbles on this from google or something, you would need to do a call method to make that work, but that's probably the wrong approach; you're better off doing something with contextlib.asynccontextmanager or similar).

It's... perhaps a bit of a silly mistake, but we all make those! When I was done enough with the refactor to start testing it, I just saw a rather unceremonious:

pipenv run python -m some_module
Traceback (most recent call last):
  File "/usr/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "...", line 114, in <module>
    trio_asyncio.run(main)
  File ".../lib/python3.7/site-packages/trio_asyncio/loop.py", line 393, in run
    trio.run(_run_task, proc, args)
  File ".../lib/python3.7/site-packages/trio/_core/_run.py", line 1769, in run
    run_impl(runner, async_fn, args)
  File ".../lib/python3.7/site-packages/trio/_core/_run.py", line 1918, in run_impl
    runner.task_exited(task, final_outcome)
  File ".../lib/python3.7/site-packages/trio/_core/_run.py", line 1404, in task_exited
    raise TrioInternalError
trio.TrioInternalError

Perplexed, I set out doing some machete mode debugging (ie, adding print statements in places I thought could be troublesome), until I finally stumbled back onto my context-managers-that-really-weren't. So I fixed those, whereupon I found... I still got the same error. So then, perplexed again, I looked at the code a second time and realized I'd forgotten to update the calling code:

# Inside *some other* context manager (I know, it's messy)
async def __aenter__(self):
    await stack.enter_async_context(foo(bar))

And that got me back up and running! But it would have been really nice if Trio could have surfaced the original problem -- which was really that I was trying to call something that wasn't callable, inside an aenter, in a stack... -- so that I could have fixed it as easily as the silly mistake that it was!

[njsmith]

Are you able to share a minimal reproducer? I don't see how any of the things you've said could create this on their own, so I'm wondering if there's some extra detail missing, like a problem with nesting nurseries or something... (The mention of the stack also makes me think of #1243; not sure if you've seen that.)

[Badg]

I've got a repro; I'm not entirely sure if it's minimal, but it works. I'm super crunched for time right now (hence not trying to eliminate stuff for a true minimal repro) so it's not clean, but it's a repro. It's 100% without context (and without any cleanup), but it's better than nothing!

import contextlib
import trio

import cachetools


class ReprotesterOuter:

    async def __aenter__(self):
        inner = ReprotesterInner(maxsize=7)
        stack = contextlib.AsyncExitStack()
        send, recv = trio.open_memory_channel(max_buffer_size=float('inf'))
        await stack.enter_async_context(recv)
        nursery = await stack.enter_async_context(trio.open_nursery())
        nursery.cancel_scope.shield = True
        nursery.start_soon(trio.sleep, 7)
        await stack.enter_async_context(inner('foo'))

    async def __aexit__(self, *args, **kwargs):
        pass


class ReprotesterInner(cachetools.LRUCache):
    pass


async def main():
    outer = ReprotesterOuter()
    async with outer:
        await trio.sleep(0)

trio.run(main)

[Badg]

I'm pretty sure cachetools is a red herring here BTW. I just threw in a bunch of stripped-down setup code and looked to see if it'd repro. Looks plausibly related to #1243

[njsmith]

I haven't debugged in detail, but a custom __aenter__ with a nursery inside it is a huge red flag – we see people getting into trouble with that all the time. If you get an exception inside the __aenter__ then the nursery __aexit__ won't run and that breaks the nursery nesting rules. Rule of thumb is to always use @asynccontextmanager or @contextmanager to define non-trivial custom context managers – it's incredibly hard to get all the error unwinding cases correct otherwise.

[njsmith]

(we should also have better error messages for nursery nesting errors... I feel like there's an issue open for that but I'm not sure where)

[Badg]

It's not possible to use asynccontextmanager in this case and satisfy our API constraints. However, that's an excellent point, and updating it to something more like this makes the terminating error the "not callable" you'd expect:

import contextlib
import trio

import cachetools


class ReprotesterOuter:

    async def __aenter__(self):
        inner = ReprotesterInner(maxsize=7)
        stack = self.stack = contextlib.AsyncExitStack()
        try:
            send, recv = trio.open_memory_channel(max_buffer_size=float('inf'))
            await stack.enter_async_context(recv)
            nursery = await stack.enter_async_context(trio.open_nursery())
            nursery.cancel_scope.shield = True
            nursery.start_soon(trio.sleep, 7)
            await stack.enter_async_context(inner('foo'))
        except BaseException:
            try:
                await stack.aclose()
            finally:
                raise

    async def __aexit__(self, *args, **kwargs):
        await self.stack.aclose()


class ReprotesterInner(cachetools.LRUCache):
    pass


async def main():
    outer = ReprotesterOuter()
    async with outer:
        await trio.sleep(0)

trio.run(main)

[Badg]

However, given that it appears the stack/nursery combination is the problem, I think it makes sense to close this as a dupe of #1243! Thanks for the inadvertent code review ;)

PS -- I realize this is a really messy way of using Trio; in general trio is an absolute pleasure to use. But -- especially in setup code with complicated API constraints -- I've run into one or two situations where I need to get a little creative like this. Fortunately it's pretty easy to constrain the madness to a single place.

Reflecting on things a bit, I think ultimately the difficulty here is with the __(a)enter__/__(a)exit__ spelling that python uses; in practice you almost always use the @(async)contextmanager decorators. But if you truly need to spell things as (async) with foo instead of (async) with foo.bar() (again, complicated API constraints) there's no way around it, which is a shame. If python instead used __(a)context__, this whole thing would be impossible!

[njsmith]

async def __aexit__(self, *args, **kwargs):
   await self.stack.aclose()

Unfortunately, this is still incorrect! The __aexit__ protocol gets to see and potentially modify any exception raised inside the body, and the nursery __aexit__ relies on that... e.g. if a background task crashes, then the nursery cancels its body, and then later captures the Cancelled exceptions. But here the Cancelled exceptions will leak out and cause havoc.

I think you could do:

async def __aexit__(self, *args):
    return self.stack.__aexit__(*args)

Your __aenter__ has a similar problem: it needs to pass the BaseException through the stack's __aexit__, and do something complicated with the return value.

You can read the full details here: https://www.python.org/dev/peps/pep-0343/#specification-the-with-statement

Reflecting on things a bit, I think ultimately the difficulty here is with the __(a)enter__/__(a)exit__ spelling that python uses; in practice you almost always use the @(async)contextmanager decorators. But if you truly need to spell things as (async) with foo instead of (async) with foo.bar() (again, complicated API constraints) there's no way around it, which is a shame. If python instead used __(a)context__, this whole thing would be impossible!

Good point. I guess one could do:

class DunderContext(abc.ABC):
    @abstractmethod
    def __context__(self):
        raise NotImplementedError

    def __enter__(self):
        self.__context = self.__context__()
        return self.__context.__enter__()

    def __exit__(self, *args):
        return self.__context.__exit__(*args)

class MyManager(DunderContext):
    @contextmanager
    def __context__(self):
        ...

(and similarly for async context managers).

[Badg]

Good point. I guess one could do:

Huh. I rather like that actually! That's a pretty nice workaround for the context spelling.