Code

Opened 6 years ago

Closed 6 years ago

#6131 closed (fixed)

Commit [6887] broke locmem (default) cache

Reported by: mtredinnick Owned by: nobody
Component: Core (Cache system) Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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 sherbang 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by jacob

I'll take a look at this...

comment:3 Changed 6 years ago by sherbang

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 Changed 6 years ago by Karen Tracey <kmtracey@…>

I believe #6153 is a dup of this.

Changed 6 years ago by sherbang

comment:5 Changed 6 years ago by sherbang

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 Changed 6 years ago by mtredinnick

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 Changed 6 years ago by sherbang

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 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [6904].

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.