Opened 4 years ago
Closed 15 months ago
#31949 closed New feature (fixed)
Allow builtin view decorators to be applied directly to async views.
Reported by: | Michael Galler | Owned by: | Ben Lomax |
---|---|---|---|
Component: | Core (Other) | Version: | 3.1 |
Severity: | Normal | Keywords: | async |
Cc: | Andrew Godwin, Ben Lomax, Roy Smith, Jon Janzen | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
The cache middleware is broken because it inherits from MiddlewareMixin and override the init method.
TypeError: object HttpResponse can't be used in 'await' expression
On both middlewares the self._async_check() is missing in init.
Also the most view decorators are broken on async views, because the dont provide async/await.
For example:
cache_control xframe_options_deny vary_on_cookie
Change History (52)
comment:1 by , 4 years ago
Description: | modified (diff) |
---|
follow-up: 3 comment:2 by , 4 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Core (Other) |
comment:3 by , 4 years ago
Replying to felixxm:
The cache middleware is broken because it inherits from MiddlewareMixin and override the init method.
An issue with the MiddlewareMixin subclasses is already handled in #31928.
I had not checked the ticket, so that's already addressed.
You should be able to chain them with
@sync_to_async()
, if necessary. Am I missing sth?
How it works for me
@sync_to_async @xframe_options_deny @async_to_sync async def get(self, request, *args, **kw): slug = kw['slug']
But I find this complicated, wouldn't it be easier if we changed the decorators as follows
def xframe_options_deny(view_func): def wrapped_view(*args, **kwargs): resp = view_func(*args, **kwargs) if resp.get('X-Frame-Options') is None: resp['X-Frame-Options'] = 'DENY' return resp return wraps(view_func)(wrapped_view)
to
def xframe_options_deny(view_func): def process_response(resp): if resp.get('X-Frame-Options') is None: resp['X-Frame-Options'] = 'DENY' return resp def wrapped_view(*args, **kwargs): resp = view_func(*args, **kwargs) return process_response(resp) async def wrapped_view_async(*args, **kwargs): resp = await view_func(*args, **kwargs) return process_response(resp) return wraps(view_func)(wrapped_view_async if asyncio.iscoroutinefunction(view_func) else wrapped_view)
This also prevents the code from being executed in a different thread, which again leads to context changes and slightly slows down the speed
comment:4 by , 4 years ago
Keywords: | async added |
---|---|
Summary: | ASGI - Cache Middleware and some view decorators broken → Allow builtin view decorators to be applied directly to async views. |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → New feature |
OK, so let's accept this as a new feature: Yes, it would be nice if the decorators could handle async views directly.
comment:5 by , 4 years ago
Yeah, while you can technically apply enough async/sync things to decorators to make them work directly, only the style that adds attributes to the wrapped function is actually going to work natively; we should at least have the core Django ones detect what they're wrapping and do the right thing, though due to Python this is impossible to do perfectly (but we can make them perfectly raise nice errors if they're not wrapped right)
comment:6 by , 4 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Picking this up to apply the solution discussed above.
comment:7 by , 4 years ago
I've opened a draft PR here that I would love some feedback on if / when anyone has a chance to take a look at it. I'm updating it as I go, but having someone cast an eye on it to make sure I'm not going down the wrong rabbit hole would be useful.
comment:8 by , 4 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Alright, I think I have a PR that's ready to check. I've left a couple of questions on the PR description, but they're not essential to the functionality (i.e. it all seems to be working).
I'm going to update the triage stage to Ready for checkin
(as I suspect it'll get more eyes on it that way) but if that's bad form please let me know and I'll happily change it back as required.
PR in question: https://github.com/django/django/pull/13483
comment:9 by , 4 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Indeed, the single person can't set Ready for checkin is the author :-)
Having patch = Yes, and all "needs ..." flags = No is sufficient to get in the review queue. A few weeks ain't bad. It'll come :-)
comment:10 by , 4 years ago
Cc: | added |
---|
Is this something that's being worked on? I've run into what I'm pretty sure is the same problem with LoginRequiredMixin (see https://stackoverflow.com/questions/66512353/using-loginrequiredmixin-with-async-views). It sounds like there's a fix that's basically ready to go, but got stalled with a merge conflict?
comment:11 by , 4 years ago
Hi Roy.
Currently this is awaiting review. If you wanted to look at the PR (the patch review checklist helps) and see what you think, that would be great.
See the Triaging Tickets docs for the general workflow — you can comment, and mark as needs tests, or needs improvement, or needs docs and so on, or set the triage state to Ready for commit, that will let Mariusz and I know that you think it's ready.
comment:12 by , 4 years ago
I took a quick look just now. I'll take a deeper dive in a few days when I get some time.
I do note that this doesn't cover django.contrib.auth.decorators.login_required. If I understand things correctly, that (and LoginRequiredMixin) depend on the DB backend, which is a whole other level of async complexity?
comment:13 by , 4 years ago
Hi Roy,
As far as I know this PR mostly just needs reviewing. I'm pretty busy for the next few days, but then I'll happily look at the merge conflicts and see if I can get it to a state where it's mergeable again. Oh, and apparently there was a comment left in December about annotations and release notes required, so I'll look to fix those up too.
It would be great to finally get a bit of momentum going on this :)
comment:14 by , 4 years ago
I'm afraid I'm not going to be as useful here as I had hoped. I got as far as verifying that the docs all build, and that everything passes flake8. Unfortunately, I don't have a proper django development environment set up, and was unable to run the tests. When I get up to:
pip install -r requirements/py3.txt
it eventually fails with:
In file included from src/_pylibmcmodule.c:34: src/_pylibmcmodule.h:42:10: fatal error: 'libmemcached/memcached.h' file not found
Unfortunately, it's going to be impractical for me to give you guys a proper review.
follow-up: 16 comment:15 by , 4 years ago
Roy, the continuous integration server takes care of running the tests for each pull request, so you don't need to do that locally. However, to remedy your error you can install libmemcached-dev
on most Unix/Linux, I think (or do a web search for that error and your OS and you should find the remedy).
Reviewing a patch is mostly about examining the content.
follow-up: 17 comment:16 by , 4 years ago
Replying to Tim Graham:
OK. In any case, I stood up a clean debian 10 box and I'm picking this up there.
comment:17 by , 4 years ago
Review is done. This is the first code review I've done for django, so I hope what I did is what you were looking for :-)
comment:18 by , 4 years ago
Thank you for your review Roy, I've updated the PR now to both deal with the merge conflict and enact proposed changes / offer explanations as to why I didn't enact them. Let me know if there's anything you disagree with and I'm happy to have another round of changes as required.
The only thing left is to add a release note, but that'll need to wait for a day or two until the current minor update is pushed out (I think it's due out tomorrow and I don't imagine the PR will get approved and merged in that time).
comment:19 by , 3 years ago
Patch needs improvement: | set |
---|
comment:20 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:21 by , 2 years ago
Patch needs improvement: | set |
---|
I'm still worried about the proposed approach on the current PR.
- It's quite complex. There's a utility function
sync_async_wrapper
which is called inside each decorator implementation, making it less readable in each case I think. - This also forces each implementation into the
process_request()
,process_response()
pattern, rather than allowing that to be inline and ties the implementation of those as sync.
Then:
- The
contrib.auth
decorators and mixins aren't handled. - Each of the decorators has the
sync_and_async_middleware()
decorator applied (marking it as both async and sync capable) but the attributes set are never checked in the code anywhere.
Marking as patch needs improvement again on that basis.
I'll continue to look at this, and try and discuss with folks over the DjangoCon period this autumn but, I think:
- In some cases (certainly for the mixins) implementing the async-switch inline in the same way as the `View.options` handler does now would be feasible. (This would solve Roy's issue from the StackOverflow thread.)
- For others, it would be worth looking at a wrapper around the decorator implementation that adjusts the wrapped callback, rather than having to call a helper. (For example,
@xframe_options_deny
&co need a sync view, currently, but can be adapted byBaseHandler
after that, if needed.)
comment:22 by , 2 years ago
Thank you for your review, it's always very appreciated. I'll try to address each of your points as best as I understand things:
- Complexity / forced implementation: The utility function was my attempt to make the decorator function work more similarly to the mixin classes, which look for
process_request
andprocess_response
attributes amongst others. However, it might be that we don't want decorator functions to mirror the decorator class pattern, and so I'm happy to move away from it.
- Sync-only
process_X
functions: As it stands, I would suggest that allowing for async passed-in functions (e.g. theetag_func
argument of the@condition
decorator) should be pushed back into a separate piece of work after this one. It seems to me that converting the builtin decorators to be async-able at the cost of only allowing sync functions to be passed to them is a good iterative step forward.
- The
contrib.auth
decorators: Fair point, I can cover those too.
sync_and_async_middleware
never checked in functions: Correct me if I've misunderstood this, but I thought that the decorator wasn't for internal use, but was used by external code as a way of knowing if the middleware could handle sync and/or async view functions. For example, when layering middleware it would help Django figure out if it needed to "adapt the request to fit the middleware's requirements" (as per [the docs](https://docs.djangoproject.com/en/4.1/topics/http/middleware/#asynchronous-support)).
- async-switch inline: I think this should be possible for any decorators which only act of the request. If a decorator needs to act on the response, we need to await the view to get the response, _and then_ act on that response. If this isn't acceptable, we might be able to do something with partial functions perhaps, but that seems like a non-trivial jump in complexity.
- implementation that adjusts the wrapped callback: I'll be honest, I don't think I understand your suggestion here. Are you suggesting we somehow flag a
process_request
function to be run when theBaseHandler
handles the async view? I think this would mean defining the response processing in the decorator, but not actually running it "within the decorator".
A good follow up question feels like is: is there a way to break this work up logically to give partial functionality quicker? For example, only doing this for decorators which don't act on the view response?
comment:23 by , 2 years ago
Hey Ben.
I think the shorter version would be that the sync_async_wrapper
strikes me as overly complex. I think it would be much easier on the eye inlining the requisite code each time. I suspect for each decorator the needed code is minimal. That wouldn't be as DRY™, but it would exhibit better locality of behaviour, and be easier to maintain.
An incremental way forward would be looking at a subset of the decorators. I suspect, the contrib.auth
decorators are the ones I'm most likely to want to use first... 🤔
comment:24 by , 2 years ago
Cc: | added |
---|
comment:25 by , 2 years ago
Hi Carlton,
Thanks for that clarification, I've now updated the PR to:
- Remove the
sync_async_wrapper
function completely; all the decorators now handle their logic "inline". I've still pulled out some of the shared code between the sync and async versions of the decorators, so you'll still see a few_process_request
and_process_response
about the place, but it's now much clearer what they actually do.
- Make all of the
contrib.auth
decorators be able to handle sync and asycn views.
The documentation update still points to version 4.2, let me know if that's no longer correct.
comment:26 by , 2 years ago
Patch needs improvement: | unset |
---|
Thanks Ben. I’ll uncheck Patch needs improvement, and give it another look
comment:27 by , 2 years ago
Has patch: | unset |
---|
Hi Ben — Happy New Year!
I've looked again at the PR, both before and after Christmas. In summary, I
think we should address this ticket in a series of smaller PRs, rather than
trying to do them all in one big go.
As I see it, there are two groups of issue with the current approach:
- The bulk edit means the code is quite a few times not sensitive enough to the individual decorator. Looking at them, couldn't we write this one that way, or that one this way?, is the thought. (I left some more specific comments on the PR itself.) If we address them one-by-one (or in small related groups, likely) we can write the best code for each case. There are repeated patterns and opportunities for code-reuse, but I think extracting them as we go is going to work better than trying to determine them in advance.
- Related, the tests for each decorator need to live with the existing related tests, not be centralised in one place. Five years hence, when we come to work on the login_required logic, say, we need to be able to open the relevants tests and see them all together. Having to track down a separate set of tests in another part of the test suite entirely would be a maintenance headache. If we address the decorators individually this tendency won't be there.
I hope both of those points make sense?
I think the general idea is correct though, and I don't see anything to stop
us being able to process smaller Refs #31949 -- ...
PRs relatively quickly.
I think it would be good to address the whole list of built-in decorators within a single release cycle though. As such I'm going to put this on my target list for 5.0. I'll pick it up mid-cycle to help make sure we can get it over the line.
Thanks again for your patience and input. I know it's frustrating when it takes a while to get it lined up right.
comment:28 by , 2 years ago
Hi Carlton, and a Happy New Year to you too 😁
Splitting up the work and moving the tests to separate files seems like a good idea to me, I'll try to get started on those in the next few weeks at the latest. The one caveat I would put forward is to keep the test_decorators_sync_and_async_capable
test (link to code) in the shared test file, as it's less of a test of the behaviour of each decorator, and in theory shouldn't change for a while anyway (i.e. none of them should go back to just being only sync or async capable). How does that sound?
comment:29 by , 2 years ago
To get the ball rolling, I've thrown together the first of these PRs here, which looks at the cache
decorators.
comment:30 by , 21 months ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:34 by , 21 months ago
Needs documentation: | set |
---|
PR for the cache decorators is nearly ready to go. (Tweaks and docs.)
comment:35 by , 21 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:37 by , 20 months ago
Patch needs improvement: | unset |
---|
comment:40 by , 19 months ago
As an update, I've got 3 PRs to contribute to this issue in various states:
- Made @xframe_options_(deny/sameorigin/exempt) decorators to work with async functions
- Reviewed once, tests are failing, but I think it might be a test runner failure rather than an actual test failure (
ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
) and I'm not sure how to re-run the tests.
- Reviewed once, tests are failing, but I think it might be a test runner failure rather than an actual test failure (
- Made @no_append_slash work with async functions
- Passing all tests, needs a review
- Made @sensitive_variables and @sensitive_post_parameters work with async functions
- Legitimately failing tests, I need to come back to it, but wanted to flag that I've started somewhere
There's going to be a little bit of conflicts with all of them due to the docs being updated in the same place, but that should be easy enough to fix once they start to get merged.
I also believe that the http
decorators are also being worked on in this PR by Th3nn3ss, so I'm leaving those alone for now.
comment:46 by , 18 months ago
Has patch: | unset |
---|---|
Patch needs improvement: | unset |
comment:47 by , 17 months ago
I've created a couple more PRs which I think cover all of the remaining non-decorator_from_middleware
decorators (and excellent work on the sensitive parameter decorators by the way 😄):
- Made @csrf_exempt decorator work with async functions
- Made @vary_on_(cookie/headers) decorators work with async functions
For the decorator_from_middleware
decorators, I've put together a proof of concept (POC) draft PR, with a few notes:
- It only implements tests for
gzip_page
, to make sure that it actually works for at least one of the affected decorators. - The second commit attempts to DRY up the code (as there is a lot of code duplication otherwise), but I'm not convinced it's actually helpful.
- I've not added any documentation.
This POC PR is meant to act as a place to have a conversation which can point to specific code changes, rather than a solution that I feel strongly about 🙂
comment:52 by , 15 months ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
An issue with the MiddlewareMixin subclasses is already handled in #31928.
You should be able to chain them with
@sync_to_async()
, if necessary. Am I missing sth?