#15144 closed (fixed)
Max age set in cache control no longer obeys timeout set with @cache_page decorator
Reported by: | Jim Dalton | Owned by: | nobody |
---|---|---|---|
Component: | Core (Cache system) | Version: | dev |
Severity: | Keywords: | blocker, regression | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Per the documentation, I would expect the max-age value set in the Cache Control to obey the timeout value passed to @cache_page
when that decorator is used in a view. However, at present the max_age ignores that setting and instead uses the default timeout from the cache. (As a matter of fact, it even ignores the CACHE_MIDDLEWARE_SECONDS settings.)
I believe this is a regression, which from what I can tell, appears to have arisen in [15021]. This changeset introduced the following changes:
The following line, which originally set the value of self.cache_timeout
at the start of CacheMiddleware.__init__
to cache_timeout
, was removed:
self.cache_timeout = cache_timeout
The following line was added to CacheMiddleware.__init__
:
self.cache_timeout = self.cache.default_timeout
The result is that max_age is set to the value of whatever the cache timeout setting of the cache used, rather than the value of cache_timeout
passed into __init__()
as it did formerly.
I wasn't able to find any background on this changeset so I couldn't determine whether this was a purposeful change in behavior for some other reason. For now, though, I can only conclude that this is a bug, since I would expect max-age to match the value of the timeout passed to cache_page.
Attachments (3)
Change History (10)
comment:1 by , 14 years ago
Keywords: | blocker regression added |
---|
by , 14 years ago
Attachment: | tests_demonstrating_regression.diff added |
---|
comment:2 by , 14 years ago
Thanks Russ. I uploaded a patch which, for me anyway, produces a failure in the test suite demonstrating the regression.
Basically, in these tests I add an additional view that sets an explicit cache_timeout of 4 seconds. I hit the view once along with the other tests to get it in the cache. After the 2 second sleep, my view with a 4-second timeout should still be hitting the cache. However, it does not, since it's using the timeout value of 1 that was put in when the 'other' cache is set up for the tests.
Also, if you look at the value of the Cache-Control header after the first hit on my view, e.g. if you do:
# Request from the alternate cache with a new prefix and a custom timeout response = other_with_timeout_view(request, '20') self.assertEquals(response.content, 'Hello World 20') print response['Cache-Control']
It'll print "max-age=1" instead of 4, which is the value passed to the view in the decorator.
I agree with you, something funky is going on, because self.cache_timeout = self.cache.default_timeout *should* work, but at least for me it doesn't. I'll observe that a trivial fix is self.cache_timeout = cache_timeout or self.cache.default_timeout. This does fix the problem, but I'm not sure it's the right answer because I think there is some other stuff that needs to be sorted out properly in init.
comment:3 by , 14 years ago
Has patch: | set |
---|
The problem is in get_cache - it does not do anything with the provided kwargs unless the specified cache is in URL format (i.e. has ':' in it). Patch attached - I'd probably recommend also including jsdalton's unit tests.
-jag
comment:4 by , 14 years ago
Okay, I'm uploading a new patch that:
- Includes the regression tests I wrote under
test_view_decorator
that demonstrating the original issue - Fixes the issue originally raised in this ticket by incorporating Joshua's patch (15144.patch).
- Includes a new test under
CacheMiddlewareTest
,test_constructor
, which highlight a few bugs in the constructor that were discussed briefly here: http://groups.google.com/group/django-developers/browse_thread/thread/1a019e9d30de5c9d - Fixes the issues uncovered by the new test via a fairly minor rewrite of the
__init__
method ofCacheMiddleware
With regards to the first issue and fix, that's pretty much been discussed above so no need to say more about it.
With regards to the second issue and fix, the main problem was that if the CacheMiddleware
class was being used as middleware, the values for some of the instance attributes were not being set properly. (Basically, the except KeyError
code path was never being executed.)
The new test should be pretty self explanatory and should raise several failures when run against the original CacheMiddleware.__init__
code currently in trunk
The rewrite is also hopefully self explanatory as well. In addition to fixing the bugs I tried my best to simplify some of the code in an effort to make it more readable and easy to maintain going forward. A few notes of clarification:
- I changed
cache_alias
to an instance attribute (i.e.self.cache_alias
) in order to make it more testable (since it's not possible to determine which alias is being used fromself.cache
itself.) - I smoothed out a lot of places where variables were being explicitly compared against None etc. I think the new test adequately ensures that the appropriate initialization behavior is taking place, and some of those constructions were (IMO) making the code a bit unnecessarily verbose and hard to read.
- I wish there was better testing to ensure the right values are being set in
cache_kwargs
and passed toget_cache()
but I think the other upstream tests should fail if any of that were going wrong so it's probably okay.
Anyhow, please let me know if there are any issues with the patch or anything else that needs to be addressed. (Apologies if I should have opened a new ticket to address those other bugs, I just figured it was easier to get it all taken care of here.)
Cheers.
Jim
by , 14 years ago
Attachment: | fix_15144_regressions.patch added |
---|
comment:5 by , 14 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
Ok - your test case now makes it clear that there are a couple of bugs. I'm marking RFC, will check in shortly with a couple of minor modifications.
In particular:
- Using try:catch logic instead of "if contains" logic is marginally faster, because it only requires one lookup for all cases, rather than two lookups on the successful lookup case.
- The or-emulate "ternary if" is prone to error, especially in the "if x is None" versus "x is False" case, so we generally prefer the explicit if statement.
comment:6 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I'm a little uncertain as to the exact use case where you are seeing this problem. Yes, the code has been changed to set self.cache_timeout = self.cache.default_timeout, but that default timeout should be the value that has been passed into the cache backend.
Can you reduce this to a relatively simple test case that demonstrates when the wrong value is being used?
Marking as a blocker, regression because if true, this is something that needs to be fixed before release; however, we need more detail before we can confirm.