dabeaz / curio

Good Curio!
Other
4.02k stars 241 forks source link

Unsafe generators and async context managers #247

Closed Fuyukai closed 6 years ago

Fuyukai commented 6 years ago

Currently, using an async context manager like so will always fail:

@asynccontextmanager
async def something():
    try:
        yield (await some_other_action())
    finally:
        await cleanup()

Trying to use the context manager would cause a RuntimeError when the generator is instantiated. The normal fix is to wrap the agen in an async with curio.meta.finalize(...) but that's not possible without creating a custom context manager.

imrn commented 6 years ago

The problem is that Python 'sync' runtime drives the "finally:" section, so that you can not have any asyncs there.

dabeaz commented 6 years ago

Need more context: where is "acontextmanager" coming from?

dabeaz commented 6 years ago

Does it work if you do this:

from curio.meta import safe_generator

@acontextmanager
@safe_generator
async def something():
      ...
Fuyukai commented 6 years ago

Oh right, acontextmanager is the 3.7 contextlib one.

And that does fix it, but that feels like lying if my generator doesn't pass the safety test and bad things could happen.

dabeaz commented 6 years ago

The safety check is really focused more on the use of async generators being used to drive async iteration---and the possibility of a for-loop breaking out early without finalizing the generator. I'm not sure this technically applies to the way in which an async context manager would be driven.

I'll poke inside the 3.7 source of acontextmanager to see if there's an easy way to detect its presence.

njsmith commented 6 years ago

Yeah, the safety test really is "is it safe to use this without a context manager", and asynccontextmanager is, by definition, a context manager :+)

njsmith commented 6 years ago

The clown nose there is a typo but I'm gonna leave it.

Fuyukai commented 6 years ago

Oh, okay, that makes sense. This issue probably makes more sense as just preventing the error from occurring from an acontextmanager then.

dabeaz commented 6 years ago

There's nothing that can't be solved with more levels of frame-hacking. Two clown noses might be warranted.

imrn commented 6 years ago

On 1/24/18, David Beazley notifications@github.com wrote:

There's nothing that can't be solved with more levels of frame-hacking.

Yes there are ways. But does that mean the user can use ordinary generator forms, or he/she should built special (split?) forms emulating try/finally etc..

dabeaz commented 6 years ago

Instead of magic, perhaps Curio should just provide its own decorator for it:

from contextlib import acontextmanager as real_acontextmanager
def acontextmanager(func):
      return real_acontextmanager(safe_generator(func))
dabeaz commented 6 years ago

Fixed. Curio now detects the use of @contextlib.asynccontextmanager.