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 Carl Meyer, 9 years ago

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.

comment:2 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:3 by Ülgen Sarıkavak, 8 months ago

Cc: Ülgen Sarıkavak added

comment:4 by Wassef Ben Ahmed, 5 months ago

Has patch: set

comment:5 by Wassef Ben Ahmed, 5 months ago

Owner: changed from nobody to Wassef Ben Ahmed
Status: newassigned

comment:6 by Sarah Boyce, 5 months ago

Patch needs improvement: set

comment:7 by Sarah Boyce, 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 Wassef Ben Ahmed, 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 Sarah Boyce, 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 Wassef Ben Ahmed, 3 months ago

Patch needs improvement: unset

comment:11 by Wassef Ben Ahmed, 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 Sarah Boyce, 3 months ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top