Opened 16 years ago

Closed 16 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)

locmem-fix.patch (2.1 KB ) - added by Brian Johnson 16 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by Malcolm Tredinnick, 16 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Jacob, 16 years ago

I'll take a look at this...

comment:3 by Brian Johnson, 16 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.

comment:4 by Karen Tracey <kmtracey@…>, 16 years ago

I believe #6153 is a dup of this.

by Brian Johnson, 16 years ago

Attachment: locmem-fix.patch added

comment:5 by Brian Johnson, 16 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 Malcolm Tredinnick, 16 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 Brian Johnson, 16 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.

comment:8 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

Fixed in [6904].

Note: See TracTickets for help on using tickets.
Back to Top