Opened 9 years ago
Last modified 3 months ago
#25782 assigned Cleanup/optimization
Discourage usage of cache_page decorator with UpdateCacheMiddleware (or make middleware ignore decorated views)
Reported by: | Serhiy | Owned by: | Wassef Ben Ahmed |
---|---|---|---|
Component: | Documentation | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
If django.middleware.cache.UpdateCacheMiddleware is active and some view is decorated with the cache_page that overrides cache alias, this view will be cached in both caches, first time by cache_page and then by the middleware. Well, at least it will try. And fail if the default cache is memcached and the view response is bigger than its' maximum entry size. While filebased cache will work and even create the cache entry. This way first user to visit when there are no entry in cache will get error instead of data.
So, I guess there should be some warning about this in the docs.
Ideally, middleware should ignore views with overridden cache alias, maybe by means of checking headers like in case of the cache_control decorator.
Change History (12)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 8 months ago
Cc: | added |
---|
comment:4 by , 5 months ago
Has patch: | set |
---|
comment:5 by , 5 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 5 months ago
Patch needs improvement: | set |
---|
comment:7 by , 4 months ago
Although I am now happy with the patch, this might rely on #15855 to be fixed and so further investigation is required in that ticket before proceeding further here
comment:8 by , 4 months ago
Patch needs improvement: | unset |
---|
@sarahboyce, update is that I backed out from pushing what's mentioned above.
I guess ticket-15855 will have to be worked on in a separate PR.
for now, I updated the release notes for the approved change.
and, Yes I confirm, UpdateCacheMiddleware still needs to be at the top for it to handle undecorated views etc.
comment:9 by , 3 months ago
Patch needs improvement: | set |
---|
Added minor comments on the PR
Before this can be marked as "Patch needs improvement" False, we need to check that this change doesn't introduce issues due to #15855
comment:10 by , 3 months ago
Patch needs improvement: | unset |
---|
comment:11 by , 3 months ago
Its not necessary to have UpdateCacheMiddleware on top, it's only a workaround suggested by ticket-15855, but let's question it?
as without FetchFromCacheMiddleware, the UpdateCacheMiddleware will be just setting the value to some cache with the correct headers but not actually serving them as the value by this decorator will be the one served... so the workaround just causes this problem and fixes nothing.
with this PR, will avoid that unnecessary extra write in the middleware, and continue having “Vary headers“ missing.
on the other hand I think the "vary header" problem is smaller to have compared to doubling cache size so we should proceed with this until someone gathers the courage to deprecate the decorator?
comment:12 by , 3 months ago
Patch needs improvement: | set |
---|
Any attempt to fix this should also take into account #15855 -- the
cache_page
decorator is already basically broken in terms of Vary headers. It shouldn't cache at all, it should mark the response for later caching by the middleware, or something.