Opened 3 years ago

Closed 16 months ago

Last modified 16 months ago

#18541 closed Bug (duplicate)

Infinite Lock With Caching Backend

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

Description

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.

Attachments (2)

LocMemlocking.diff (1.9 KB) - added by zmsmith 3 years ago.
LocMemlocking.diff
LocMem.diff (2.1 KB) - added by zmsmith 3 years ago.
Patch with improved test

Download all attachments as: .zip

Change History (9)

Changed 3 years ago by zmsmith

LocMemlocking.diff

comment:1 Changed 3 years ago by aaugustin

  • Component changed from Uncategorized to Core (Cache system)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

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 signal.alarm()? Any better ideas?

comment:2 Changed 3 years ago by zmsmith

Had a chance to look at this, but 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 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.

Changed 3 years ago by zmsmith

Patch with improved test

comment:3 Changed 3 years ago by zmsmith

I changed the test to not create the locking behavior and fail correctly against the old code. New patch is uploaded.

comment:4 Changed 2 years ago by FrankBie

  • Needs documentation set

Testcode and your patch needs some inline comments

comment:5 Changed 2 years ago 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

comment:6 Changed 16 months ago by timo

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

Duplicate of #20613, I'll merge the test that's included in this PR though as tests weren't added with that ticket.

comment:7 Changed 16 months ago by Tim Graham <timograham@…>

In 6c5a30b4e7f51e8c255dc104714d5748e5b5870c:

Added tests for LocalMemCache deadlocks. refs #20613 and refs #18541.

Thanks Zach Smith for the patch.

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