Code

Opened 6 years ago

Closed 6 years ago

#6413 closed (fixed)

Deadlock while culling of cache using locmem backend

Reported by: Joe Holloway <jholloway7+django@…> Owned by: permon
Component: Core (Cache system) Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I'm playing around with the locmem cache backend and I set my 'max_entries' to a particularly low value such that the culling algorithm will kick in and prune my cache.

ORG_CACHE = cache.get_cache("locmem:///?max_entries=20&timeout=3600")

When the cull logic actually fires, I experience what appears to be a deadlock condition, on this line of the locmem backend:

def _cull(self):
    if self._cull_frequency == 0:
        self._cache.clear()
        self._expire_info.clear()
    else:
        doomed = [k for (i, k) in enumerate(self._cache) if i % self._cull_frequency == 0]
        for k in doomed:
            self.delete(k)  

def delete (self, key):
    self._lock.writer_enters()  # <------ deadlocks here
    try:
        self._delete(key)
    finally:
        self._lock.writer_leaves()

Based on the way the surrounding code is written, I think the '_cull' method should actually be calling the '_delete' method instead of 'delete' as the lock has already been acquired earlier in the call stack. Indeed, when I change it to call the '_delete' method, the deadlock condition no longer occurs.

Attachments (1)

locmem_6413.diff (655 bytes) - added by Joe Holloway <jholloway7+django@…> 6 years ago.
Patch for ticket #6413

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by Joe Holloway <jholloway7+django@…>

Patch for ticket #6413

comment:1 Changed 6 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

You're probably right... I'll pass to a design decision to let the smarty-heads look at it.

Perhaps bring this up on django-dev to get some confirmation.

comment:2 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 6 years ago by Joe Holloway <jholloway7+django@…>

Here's a pretty simple test case.

>>> from django.core.cache import get_cache
>>> cache = get_cache ('locmem:///?max_entries=20')
>>> for x in range (25):
...    print x
...    cache.set (x, 'foo')  # loop will hang on iteration x=20
... 

I think my patch will fix the locmem cache, but this also indicates non-intuitive behavior in django.utils.synch.RWLock. I'm not a foremost expert on threading libraries, but in my experience with similar locking mechanisms, they typically don't deadlock a thread that tries to acquire the same lock more than once.

comment:4 follow-up: Changed 6 years ago by fredd4@…

I don't know, why it is in stage "design decision needed". This is no-brainer. It is just a typo.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by permon

Replying to fredd4@gmail.com:

I don't know, why it is in stage "design decision needed". This is no-brainer. It is just a typo.

I agree, probably this ticket should be closed and patched. Furthermore open a design decision discussion about changing RWLock semantics. I personally think that Joe Holloway is correct.

comment:6 in reply to: ↑ 5 Changed 6 years ago by permon

  • Owner changed from nobody to permon
  • Status changed from new to assigned
  • Triage Stage changed from Design decision needed to Ready for checkin

Replying to permon:

Replying to fredd4@gmail.com:

I don't know, why it is in stage "design decision needed". This is no-brainer. It is just a typo.

I agree, probably this ticket should be closed and patched. Furthermore open a design decision discussion about changing RWLock semantics. I personally think that Joe Holloway is correct.

Design was enlightened in http://groups.google.cz/group/django-developers/browse_thread/thread/a8e8d9b84f525d7/d3603fcc4fd388cf?lnk=gst&q=RWLock#d3603fcc4fd388cf
So I think, that patch i sufficient.

comment:7 Changed 6 years ago by mtredinnick

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

(In [8090]) Fixed #6413 -- Fixed a deadlock situation in the locmem culling implementation.
Thanks, Joe Holloway.

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.