New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add route hooks #3

Merged
merged 15 commits into from Nov 21, 2018

Conversation

Projects
None yet
2 participants
@florimondmanca
Copy link
Member

florimondmanca commented Nov 18, 2018

  • Refactor the view layer to facilitate addition of hooks.
  • Implement registration of hooks executed just before or just after a view is executed.

Fixes #15

florimondmanca added some commits Nov 18, 2018

florimondmanca added some commits Nov 18, 2018

@alin23

This comment has been minimized.

Copy link
Contributor

alin23 commented Nov 20, 2018

Hi @florimondmanca ! Bocadillo looks like a great project and I've already started using it in two of my projects!
This hooks feature would be really useful, but I think it would be even more useful if the hooks would be allowed to be async.

A few use cases I could think of:

  • Validating the request body using jsonschema or marshmallow
  • Checking for authentication or other things that require accessing a database using async layers (for example I use asyncpg to check the database and this would not be possible in a sync hook)

The hook doesn't have to always be async, but it can be allowed to be using iscoroutine.
The same could be applied to the middleware on_before_dispatch and on_after_dispatch methods since they are called inside an async function.

What do you think?

@florimondmanca

This comment has been minimized.

Copy link
Member Author

florimondmanca commented Nov 20, 2018

Hi @alin23, very good points about async support for these hooks (and middleware!). As it is now, hooks won't allow to read the request body because it can only be through await req.body, await req.json(), etc.

The support of both async and sync functions becomes a recurring problem in the code base, I'd also like to find a more generic (yet efficient) way of dealing with it.

As for your point concerning database calls — I've just added #4 about support for databases and an ORM within Bocadillo. You seem to have valuable input concerning databases in an async context, so feel free to check out the issue. :-)

I'll mark these as needing revision and come back to it soon.

@alin23

This comment has been minimized.

Copy link
Contributor

alin23 commented Nov 20, 2018

Oh man, finally an ORM that uses asyncpg behind! I wish tortoise existed half a year ago when I had to convert all the db interactions in Noiseblend to async.

Yes, #4 would also be a great addition if tortoise would be considered instead of orator. A sync ORM is almost impossible to use in the context of an async/single-thread framework because every database call would halt every request handling until the call finishes. Of course, the db calls can be made in separate threads but as you probably know, Python doesn't have real parallelization when it comes to threads.

Anyway, the db point was just one of many examples. I could think of others like caching with aioredis or collecting metrics using aioinflux etc. The point is that with such a simple change, the hooks and middlewares would become infinitely more useful ^_^

I forked your repo and made some preliminary changes for the hooks here: alin23@66b3bb0#diff-17d3d0ba7be943aecedec66175af840c

@alin23

This comment has been minimized.

Copy link
Contributor

alin23 commented Nov 20, 2018

By the way, I have yet to find an elegant way to merge sync and async codebases. Most of the time I had to split a whole project in 2 separate sync and async codebases to keep the code readable. Let me know if you think of a better solution.

@alin23

This comment has been minimized.

Copy link
Contributor

alin23 commented Nov 21, 2018

@florimondmanca
I think I found a simple way to deal with sync functions:

import asyncio

from starlette.concurrency import run_in_threadpool


async def ensure_async(func, *args, **kwargs):
    if asyncio.iscoroutinefunction(func):
        return await func(*args, **kwargs)
    return await run_in_threadpool(func, *args, **kwargs)

# Example usage
await ensure_async(self.hooks[BEFORE], request, response, view, kwargs)

From what I see, asgiref.sync.sync_to_async does the same thing, but in a more permanent way. This little function above removes the need to check if a function is async before calling it and also doesn't block the current thread.

What do you think? Can we use this to land this PR? It's quite an important feature, I've already started using async hooks and middleware as it seems to be the best solution for most server needs.

florimondmanca added some commits Nov 21, 2018

@florimondmanca

This comment has been minimized.

Copy link
Member Author

florimondmanca commented Nov 21, 2018

@alin23 Thanks for the tip about the reusable sync -> async converter :) I used it to add support for async hooks. Async middleware will be implemented in a future PR (this is a separate issue).

Note that, although it was already written that way in the docs, I removed the view parameter from hooks because we can't provide a uniform view object/function (views undergo a lot of wrapping between the moment they're declared and the moment they're available to hooks).

If you have any comments, feel free. :) I'll probably merge this tonight.

@alin23

This comment has been minimized.

Copy link
Contributor

alin23 commented Nov 21, 2018

Awesome! No problem about the view parameter, I didn't find a use case for it anyway.

@florimondmanca florimondmanca merged commit 5e447a6 into master Nov 21, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@florimondmanca florimondmanca deleted the feature/hooks branch Nov 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment