Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#21200 closed Bug (fixed)

Inconsistent handling of "PickleError" between cache backends

Reported by: t.chaumeny@… Owned by: Florian Apolloner <florian@…>
Component: Core (Cache system) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It looks like a PickleError raised while caching some object will get caught when using LocMemCache (see https://github.com/django/django/blob/728548e483a5a3486939b0c8e62520296587482e/django/core/cache/backends/locmem.py#L30) whilst it won't when using DatabaseCache (for instance, see https://github.com/django/django/blob/728548e483a5a3486939b0c8e62520296587482e/django/core/cache/backends/db.py#L114).

I can see no valid reason why the behavior regarding pickle errors should differ among different cache backends and this can lead to errors when tests do not use the same backend as production (it just happened to me with a pickle error : a class that defines __slots__ without defining __getstate__ cannot be pickled)

Attachments (3)

locmem_django_issue_21200.patch (2.0 KB) - added by Thomas C 10 years ago.
locmem_django_issue_21200_with_tests.patch (3.5 KB) - added by Thomas C 10 years ago.
Same as above, with tests
release_note_issue21200.patch (1.0 KB) - added by Thomas C 10 years ago.
Release note patch (backwards incompatible change)

Download all attachments as: .zip

Change History (15)

comment:1 Changed 10 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 Changed 10 years ago by Baptiste Mispelon

Looking at the history of locmem.py, I found commit 76ee39ce14b851a8247c3111de0fbc91db4de1b1 which seems relevant to this issue (it's a fix for #20613).

If I understand correctly, it actually changed the handling of a PickleError (from return True to return False) and I'm not sure if this was intended or not.

comment:3 Changed 10 years ago by Thomas C

Just to clarify the issue: several cache backends rely on pickle.dumps() to serialize some object in order to save them (in database, live memory, etc.). This method can raise an exception when the object cannot be pickled, which can happen in some circumstances.

The problem is that with the LockMemCache, when such an error occurs, the cache fails silently whereas it raises a PickleError with some other caches (e.g. DatabaseCache). This behavior was introduced a long time ago by ae7f04caab1b4f2a2b509b036499e4e042caaac6.

This is dangerous because someone using LockMemCache could have systematic cache failures going silent.

In my opinion, the correct behavior is the one of DatabaseCache since those failures should be detectable and handled by users. Maybe writing specific serialize/deserialize methods whith specific exceptions could help enforcing the same behavior across backends ?

Last edited 10 years ago by Thomas C (previous) (diff)

Changed 10 years ago by Thomas C

comment:4 Changed 10 years ago by Thomas C

The patch above makes LocMemCache stop failing silently in case of pickling error, which is consistent with other backends.

comment:5 Changed 10 years ago by Thomas C

Has patch: set

comment:6 Changed 10 years ago by Tim Graham

Needs documentation: set
Needs tests: set

Could you add a regression test that demonstrates PickleError is raised when using LocMemCache. It probably also warrants a mention in the release notes as a backwards incompatible change.

Changed 10 years ago by Thomas C

Same as above, with tests

comment:7 Changed 10 years ago by Thomas C

Needs tests: unset

I added some tests covering every backend and failing specifically for the LocMemCache without the included modification.

Changed 10 years ago by Thomas C

Release note patch (backwards incompatible change)

comment:8 Changed 10 years ago by Thomas C

Needs documentation: unset

comment:9 Changed 10 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:10 Changed 10 years ago by Aymeric Augustin

Owner: Aymeric Augustin deleted
Status: assignednew

Handing this ticket over to apollo13 :P

comment:11 Changed 10 years ago by Florian Apolloner <florian@…>

Owner: set to Florian Apolloner <florian@…>
Resolution: fixed
Status: newclosed

In e112654fc81ddb3fbffbb8382b004d69367a85fe:

Fixed #21200 -- Consistantly raise errors across all cache backends.

Thanks to tchaumeny for the patch.

comment:12 Changed 8 years ago by Tim Graham

The above commit only included testes and docs -- cf7ddc576583a15dec0800f4146c2e979795bf30 included the code changes.

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