[Django] #18541: Infinite Lock With Caching Backend
Django
-------------------------------+---------------------
Reporter: zmsmith | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.4
Severity: Normal | Keywords: caching
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+---------------------
Basically, if you try to cache an object with a {{{__getstate__}}} method
that hits the cache, the LocMem backend will enter an infinite lock.
I discovered this when a QuerySet that was using a caching manager was
being written to the cache and pickling forced the QuerySet to evaluate
and interact with the cache.
The test I added covers this scenario, but unfortunately the failure
condition is an infinite lock that will just hang and give no feedback
about the failing test. I didn't know if django has any convention for
timing out tests, so I didn't try to create one.
--
Ticket URL: <https://code.djangoproject.com/ticket/18541>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: zmsmith | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.4
Severity: Normal | Resolution:
Keywords: caching | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by aaugustin):
* needs_better_patch: => 1
* component: Uncategorized => Core (Cache system)
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
Historically, a one-liner put in the cache a deepcopy of the object. Later
deepcopy was replaced by pickle, and the code was refactored to split the
two operations:
- creating acopy of the object
- storing it in the cache
Only the second part needs to be thread-safe. I agree with the change.
An test that hangs infinitely isn't helpful. Maybe we could put a timeout
with [http://docs.python.org/library/signal#signal.alarm signal.alarm()]?
Any better ideas?
--
Ticket URL: <https://code.djangoproject.com/ticket/18541#comment:1>
Django
Reporter: zmsmith | Owner: nobody
Component: Core (Cache system) | Version: 1.4
Severity: Normal | Resolution:
Keywords: caching | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Comment (by zmsmith):
Had a chance to look at this, but
[http://docs.python.org/library/signal#signal.alarm signal.alarm()] is not
cooperating with the thread locking.
Interestingly, when this lock occurs, {{{SIGINT}}} is also just getting
swallowed. I wonder if the code in
[https://github.com/django/django/blob/master/django/utils/synch.py
django.utils.synch] is doing something a little too aggressive, but it's
hard for me to say because I don't have any real experience with
threading.
--
Ticket URL: <https://code.djangoproject.com/ticket/18541#comment:2>
Django
Reporter: zmsmith | Owner: nobody
Component: Core (Cache system) | Version: 1.4
Severity: Normal | Resolution:
Keywords: caching | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Comment (by zmsmith):
I changed the test to not create the locking behavior and fail correctly
against the old code. New patch is uploaded.
--
Ticket URL: <https://code.djangoproject.com/ticket/18541#comment:3>
Django
Reporter: zmsmith | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.4
Severity: Normal | Resolution:
Keywords: caching | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by FrankBie):
* needs_docs: 0 => 1
Comment:
Testcode and your patch needs some inline comments
--
Ticket URL: <https://code.djangoproject.com/ticket/18541#comment:4>
Django
Reporter: zmsmith | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 1.4
Severity: Normal | Resolution:
Keywords: caching | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Pull request exists as well (I think it's the same as the patch attached
to the ticket): https://github.com/django/django/pull/694
--
Ticket URL: <https://code.djangoproject.com/ticket/18541#comment:5>
Django
Reporter: zmsmith | Owner: nobody
Type: Bug | Status: closed
Component: Core (Cache system) | Version: 1.4
Keywords: caching | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* status: new => closed
* resolution: => duplicate
Comment:
Duplicate of #20613, I'll merge the test that's included in this PR though
as tests weren't added with that ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/18541#comment:6>
Django
Reporter: zmsmith | Owner: nobody
Type: Bug | Status: closed
Component: Core (Cache system) | Version: 1.4
Severity: Normal | Resolution: duplicate
Keywords: caching | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6c5a30b4e7f51e8c255dc104714d5748e5b5870c"]:
{{{
#!CommitTicketReference repository=""
revision="6c5a30b4e7f51e8c255dc104714d5748e5b5870c"
Added tests for LocalMemCache deadlocks. refs #20613 and refs #18541.
Thanks Zach Smith for the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18541#comment:7>