Commit [6887] broke locmem (default) cache

[Malcolm Tredinnick]

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.

[Jacob]

I'll take a look at this...

[Brian Johnson]

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.

[Karen Tracey <kmtracey@…>]

I believe #6153 is a dup of this.

[Brian Johnson]

The attached patch is my attempt at fixing this. add() worked-out well, but I don't like making has_key() this complex.

[Malcolm Tredinnick]

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.

[Brian Johnson]

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.

[Malcolm Tredinnick]

Fixed in [6904].