falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

Hook context managers #1953

Open adriangb opened 3 years ago

adriangb commented 3 years ago

Context managers are an expressive way to do something with cleanup. Many libraries provide functionality via context managers (for example tracing with some_lib.start_span(...) as span: ...), and users probably want to (or at least should want to) write things using context managers as much as possible.

Currently, hooks only support disconnected before/after events. The docs suggest using request.context to pass information between them, which does work, but look at the following:

@contextmanager
def do_something(req, resp, resource, params) -> None:
    yield

def do_before_wrapper(req, resp, resource, params): -> None:
    req.context._do_something_cm = do_something(req, resp, resource, params)
    req.context._do_something_cm.__enter__()

def do_after_wrapper(req, resp, resource, params): -> None:
    cm = req.context._do_something_cm
    cm.__aexit__(None, None, None)

@falcon.before(do_before_wrapper)
@falcon.after(do_after_wrapper)

And you still need to figure out error handling (maybe adding an error handler that checks for req.context._do_something_cm? Maybe this can be cleaned up using an ExitStack? Still messy).

Please let me know if there is a better way to do this, but as far as I can tell there is not.

I would propose that you implement the following:

@contextmanager
def do_something(req, resp, resource, params) -> None:
    yield

@falcon.hook(do_something)

Which takes care of all of the above boilerplate, does error handling (by letting the cm handle the error), etc.

vytas7 commented 3 years ago

Hi @adriangb ! It's probably something we could start off with a documentation recipe / FAQ improvements.

Falcon responders are normal Python methods, and you are not forced to use hooks to decorate them.

You can simply write a Python decorator, using a context manager or otherwise, e.g.:

import functools

import falcon

# TODO: Substitute with a MOTD file instead of this source code.
GREETING_FILE = __file__

def with_file(func):
    @functools.wraps(func)
    def wrapper(obj, req, resp, **kwargs):
        with open(GREETING_FILE) as fp:
            func(obj, req, resp, fp, **kwargs)

    return wrapper

class Greeter:
    @with_file
    def on_get(self, req, resp, fp):
        resp.text = fp.read()
        resp.content_type = falcon.MEDIA_TEXT

app = falcon.App()
app.add_route('/', Greeter())

But as said, I agree it may be not obvious for the user they can use any Python decorator; let's leave the issue open in terms of documentation improvement.

adriangb commented 3 years ago

Thank you for the quick reply!

Well, yes, you're right, you could always make your own decorators. However, saying that users should do that kind of invalidates the value provided by the existing decorators (just my opinion).

Also, it gets more complicated it you want to support resource level decorators: presumably now you have to parse function names on_get, on_post, etc. which I assume the built in hooks take care of.

So why not provide a decorator that accepts context managers, or alternatively let the existing hooks detect if they are being called with a context manger (via inspect) and react accordingly?

adriangb commented 3 years ago

Checking if something is a generator is pretty easy:

def is_gen_callable(call: Any) -> bool:
    if inspect.isgeneratorfunction(call):
        return True
    call = getattr(call, "__call__", None)
    return inspect.isgeneratorfunction(call)

Or if you want to accept wrapped context managers instead:

def is_contextmanager(call: Callable) -> bool:
    return hasattr(call, "__enter__") and hasattr(call, "__exit__")
vytas7 commented 3 years ago

Fair enough, we could make the hook machinery more reusable in case one wants to apply a custom decorator to a whole class.

Checking for a context manager is far from this trivial though, you need to consider various edge cases like stuff implemented in Cython, C etc. Also async bits are harder to inspect properly.

adriangb commented 3 years ago

you need to consider various edge cases like stuff implemented in Cython, C etc.

Well if someone is writing a C context manager, they can probably figure out how to wrap it in a dummy Python context manager to make it work.

Also async bits are harder to inspect properly.

Libraries like fastapi do it without any problems 🤷

If you don't want to inspect, you can always add a context_manager={True,False} parameter to the hooks. Or just make it a separate hook.

CaselIT commented 3 years ago

Libraries like fastapi do it without any problems 🤷

Do you have a link to the code or documentation for this so we can use their implementation as reference?

vytas7 commented 3 years ago

Libraries like fastapi do it without any problems

Yes, FastAPI has made different design decisions, for instance, it chooses to automatically schedule non-coroutine functions in a threadpool executor and pretend it is still performant, also the mentioned simplified inspection. I'm not saying it's necessarily wrong to do what FastAPI does; but still, we try to avoid any complex magic as much as possible. And when we do cut corners in some complex cases, we try to be crystal clear about that in documentation.

I have otherwise relabeled this issue as an enhancement proposal, so we'll see what we can do about it.

vytas7 commented 3 years ago

@adriangb Thanks for mentioning FastAPI though, is this roughly what you are after: https://fastapi.tiangolo.com/tutorial/dependencies/dependencies-with-yield/?h=context+manager#using-context-managers-in-dependencies-with-yield ?

That's not exactly direct use of context managers per se, it's just being able to use them for FastAPI's generator "dependencies" yielding a single value (IIRC pytest fixtures with cleanup can work in a similar way).

adriangb commented 3 years ago

Yes, that is what I was thinking of when I said "FastAPI can inspect callable and decide if they're sync/async context managers". Internally, FastAPI wraps the "dependency with yield" with a context manager: https://github.com/tiangolo/fastapi/blob/1760da0efa55585c19835d81afa8ca386036c325/fastapi/dependencies/utils.py#L449-L464

I'm not sure why FastAPPI went with "generator w/ a single yieldinstead of "a context manager" (my guess: to save users from having tofrom contextlib import contextmanager ... @contextmanger` everywhere, but I personally would rather avoid that magic and have the explicit imports).

But that's a bit besides the point, my point was that it is possible to inspect the callable provided by the user, although I totally understand why (and mostly agree with) not wanting to do that and instead going with an explicit mechanism via parameters or using a separate decorator.

vytas7 commented 3 years ago

(Off topic, on why it's harder to inspect async stuff. IIRC the example comes from Cython's creator, but I don't have the reference right now.

Consider your want to know whether factory() is a coroutine function or not:

def factory():
    return method()

async def method():
    # TODO: implement some stuff
    pass

Apart from disassembling bytecode, the only way to discover that factory() is, in fact, a coroutine function (and can be used everywhere where a coroutine function can be used bar cases relying on inspection) is actually calling it and inspecting the return value.)

adriangb commented 3 years ago

To be clear, I'm not saying it's without rough edges, nor am I advocating for inspecting (I usually advocate against runtime inspection).

For coroutines, you can use inpsect.iscoruotinefunction, although as you say that may not work w/ a Cython async def (I have no idea, haven't tried it) and the only 100% reliable way is to do something like result_or_coro = call(..); if inspect.isawaitable(result_or_coro): result = await result_or_coro

vytas7 commented 3 years ago

Another open issue (that unfortunately applies to after hooks as well) is that the responder's code does not always exit directly when exiting out of responder (i.e., when a context manager's __exit__ or an after hook is invoked); see also: https://github.com/falconry/falcon/issues/1505.

If the responder in question sets resp.stream, resp.sse, etc, part of the response rendition might occur outside of the context, making it confusing wrt resource cleanup. But as said, the problem does apply to after hooks as well.

CaselIT commented 3 years ago

(Off topic, on why it's harder to inspect async stuff. IIRC the example comes from Cython's creator, but I don't have the reference right now.

Consider your want to know whether factory() is a coroutine function or not:

def factory():
    return method()

async def method():
    # TODO: implement some stuff
    pass

Apart from disassembling bytecode, the only way to discover that factory() is, in fact, a coroutine function (and can be used everywhere where a coroutine function can be used bar cases relying on inspection) is actually calling it and inspecting the return value.)

at a certain point we can just be optimist and assume the user has passed the correct thing, hoping that unit-tests are used and/or a manual try was done before deploying to prod :)