[Django] #21200: Inconsistent handling of "PickleError" between cache backends
Django
-------------------------------------+--------------------
Reporter: t.chaumeny@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Cache system) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+--------------------
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`)
--
Ticket URL: <https://code.djangoproject.com/ticket/21200>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* needs_better_patch: => 0
* needs_docs: => 0
* type: Uncategorized => Bug
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:1>
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:2>
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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 an `PickleError` with some
other caches (e.g. `DatabaseCache`). This behavior was introduced a long
time ago by
[https://github.com/django/django/commit/ae7f04caab1b4f2a2b509b036499e4e042caaac6
#diff-2060839a7f833cb85ea21530ca37887aR45
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 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:3>
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tchaumeny):
The patch above makes `LocMemCache` stop failing silently in case of
pickling error, which is consistent with other backends.
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:4>
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tchaumeny):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:5>
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_docs: 0 => 1
* needs_tests: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:6>
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by tchaumeny):
* needs_tests: 1 => 0
Comment:
I added some tests covering every backend and failing specifically for the
LocMemCache without the included modification.
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:7>
Django
Reporter: t.chaumeny@… | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tchaumeny):
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:8>
Django
Reporter: t.chaumeny@… | Owner: aaugustin
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* owner: nobody => aaugustin
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:9>
Django
Reporter: t.chaumeny@… | Owner:
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* owner: aaugustin =>
* status: assigned => new
Comment:
Handing this ticket over to apollo13 :P
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:10>
Django
Reporter: t.chaumeny@… | Owner: Florian
Type: Bug | Apolloner <florian@…>
Component: Core (Cache system) | Status: closed
Severity: Normal | Version: master
Keywords: | Resolution: fixed
Has patch: 1 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Florian Apolloner <florian@…>):
* owner: => Florian Apolloner <florian@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"e112654fc81ddb3fbffbb8382b004d69367a85fe"]:
{{{
#!CommitTicketReference repository=""
revision="e112654fc81ddb3fbffbb8382b004d69367a85fe"
Fixed #21200 -- Consistantly raise errors across all cache backends.
Thanks to tchaumeny for the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:11>
Django
Reporter: t.chaumeny@… | Owner: Florian
Type: Bug | Status: closed
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
The above commit only included testes and docs --
cf7ddc576583a15dec0800f4146c2e979795bf30 included the code changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/21200#comment:12>