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 by Aymeric Augustin, 10 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 by Baptiste Mispelon, 10 years ago

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 by Thomas C, 10 years ago

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 an 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 ?

Version 0, edited 10 years ago by Thomas C (next)

by Thomas C, 10 years ago

comment:4 by Thomas C, 10 years ago

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

comment:5 by Thomas C, 10 years ago

Has patch: set

comment:6 by Tim Graham, 10 years ago

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.

by Thomas C, 10 years ago

Same as above, with tests

comment:7 by Thomas C, 10 years ago

Needs tests: unset

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

by Thomas C, 10 years ago

Release note patch (backwards incompatible change)

comment:8 by Thomas C, 10 years ago

Needs documentation: unset

comment:9 by Aymeric Augustin, 10 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:10 by Aymeric Augustin, 10 years ago

Owner: Aymeric Augustin removed
Status: assignednew

Handing this ticket over to apollo13 :P

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

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 by Tim Graham, 8 years ago

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

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