Opened 17 years ago
Closed 17 years ago
#6131 closed (fixed)
Commit [6887] broke locmem (default) cache
Reported by: | Malcolm Tredinnick | Owned by: | nobody |
---|---|---|---|
Component: | Core (Cache system) | Version: | dev |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Running the tests (particularly the "cache" test) with no particular setting for the CACHE_BACKEND shows a failure. I'm not completely up to speed with the intent of the change in [6887] so I'll let somebody more qualified or motivated figure this one out.
Attachments (1)
Change History (9)
comment:1 by , 17 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 17 years ago
comment:3 by , 17 years ago
This failure is the result of new tests that I added. I admit I only tested them against filebased as that is what I was working on.
Some history:
This stemmed from a bug Paul Bissex and I found during our work on the sprint.
get() will check the cache for expired items and delete them if found, but if you run add() or has_key() don't delete expired items, and act like they are not expired. (This showed up in strange ways in test_add sometimes). So if you have an expired item in the cache and run has_key() it will return "True" but then get() will return "None".
I included in my patch (that became [6887]) fixes for this for filebased (simply made add() and has_key() delete expired cache items), but didn't check the other cache modules.
by , 17 years ago
Attachment: | locmem-fix.patch added |
---|
comment:5 by , 17 years ago
The attached patch is my attempt at fixing this. add() worked-out well, but I don't like making has_key() this complex.
comment:6 by , 17 years ago
Something isn't right here; this looks too complicated (in has_key()
, as you suspect). The only way out of the first try...finally
block that isn't a return
is when exp < now
. So it looks like the only way to get to line 104 is when should_delete
is True. In passing, the get()
method seems to have the same problem.
So we should be able to save one if-test and a level of indentation here, for a start. Other tiny improvements would be to get the expiry time before computing now
. If it's None, you save a computation (no call to time()
). Otherwise, you can just test if exp >= time.time():
and save a variable assignment to now
and another one to should_delete
.
comment:7 by , 17 years ago
Yes, I copied that code from get(). It looks like the logic here is so that get() or has_key() only obtains a read-lock unless it needs to clean expired cache entries.
It's possible to remove the delete part from has_key() which would simplify things IE:
self._lock.reader_enters() try: now = time.time() exp = self._expire_info.get(key) if exp is None or exp < now: return False else: return True finally: self._lock.reader_leaves()
Then you rely on get() or add() to clean up the expired entries. Although if has_key() returns False then it's unlikely get() will be called anytime soon, so expired entries will hang around a lot longer.
The other option is to do away with the should_delete variable, but that really doesn't reduce the code much and ends-up making things less clear.
The more I think about it the more I think it should be added as-is.
I'll take a look at this...