#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: master
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 tchaumeny 22 months ago.
locmem_django_issue_21200_with_tests.patch (3.5 KB) - added by tchaumeny 22 months ago.
Same as above, with tests
release_note_issue21200.patch (1.0 KB) - added by tchaumeny 21 months ago.
Release note patch (backwards incompatible change)

Download all attachments as: .zip

Change History (14)

comment:1 Changed 22 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:2 Changed 22 months ago by bmispelon

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 22 months ago by tchaumeny

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 22 months ago by tchaumeny (previous) (diff)

Changed 22 months ago by tchaumeny

comment:4 Changed 22 months ago by tchaumeny

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

comment:5 Changed 22 months ago by tchaumeny

  • Has patch set

comment:6 Changed 22 months ago by timo

  • 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 22 months ago by tchaumeny

Same as above, with tests

comment:7 Changed 22 months ago by tchaumeny

  • Needs tests unset

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

Changed 21 months ago by tchaumeny

Release note patch (backwards incompatible change)

comment:8 Changed 21 months ago by tchaumeny

  • Needs documentation unset

comment:9 Changed 21 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:10 Changed 21 months ago by aaugustin

  • Owner aaugustin deleted
  • Status changed from assigned to new

Handing this ticket over to apollo13 :P

comment:11 Changed 21 months ago by Florian Apolloner <florian@…>

  • Owner set to Florian Apolloner <florian@…>
  • Resolution set to fixed
  • Status changed from new to closed

In e112654fc81ddb3fbffbb8382b004d69367a85fe:

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

Thanks to tchaumeny for the patch.

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