Opened 5 years ago

Closed 5 years ago

#16378 closed Bug (fixed)

locmem should use pickle.HIGHEST_PROTOCOL to match memcached et. al.

Reported by: Paul McMillan Owned by: Paul McMillan
Component: Core (Cache system) Version: master
Severity: Release blocker Keywords:
Cc: kubo@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


Currently the locmem cache backend does not specify a pickle protocol, meaning it defaults to the first version. There are subtle differences between the protocol versions, with some objects pickleable in one version but not another (see #15863). Since HIGHEST_PROTOCOL is what we expect to use in production, we should make sure that all the caching backends behave the same way.

Attachments (1)

16378.diff (2.1 KB) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by Aymeric Augustin

Easy pickings: set
Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Python's docs give no hint that some objects may be serializable with protocol 0 but not with protocol 2; if this occurs, it's a bug in Python. For this reason, I think we can change the protocol — it should have been set to pickle.HIGHEST_PROTOCOL in the first place — and we don't need to mention it in the release notes.

Attached patch makes all calls to pickle.dumps consistent.

The constant 2 was used in django/core/cache/backends/ It's been there "forever" (> 5 years). Until Python 2.3 pickle.HIGHEST_PROTOCOL didn't exist; it must have been common practice at the time to use 2. I didn't find any reason why we shouldn't switch to protocol 3, if it was ever introduced, so I put pickle.HIGHEST_PROTOCOL instead.

Changed 5 years ago by Aymeric Augustin

Attachment: 16378.diff added

comment:2 Changed 5 years ago by Raphael Kubo da Costa

Cc: kubo@… added

comment:3 Changed 5 years ago by Adam Nelson

Has patch: unset
Triage Stage: AcceptedUnreviewed
Version: 1.3SVN

It seems like this ticket should be expanded to change all uses of pickle.dumps() rather than just locmem and dbcache - neither of which are ever used by performance-sensitive people anyway.

comment:4 Changed 5 years ago by Paul McMillan

Triage Stage: UnreviewedAccepted

@adamnelson: Changing a ticket from Accepted to Unreviewed isn't usually done unless the original acceptance was grossly wrong.

I agree with the expansion of the scope - Django should use pickle.HIGHEST_PROTOCOL everywhere it uses pickle.

The reason this ticket was opened re: locmem and dbcache was not about performance, but consistency. In the current configuration, it's possible to have code that works just fine in testing with locmem that mysteriously stops working in production on memcached.

comment:5 Changed 5 years ago by Adam Nelson

There's no other solution to this situation with the current triage process.

Anyway, it looks like we're in agreement. I only expanded the scope because it didn't make sense to talk about consistency while at the same time having hard coded protocol numbers (or default values) scattered around the codebase.

comment:6 Changed 5 years ago by Paul McMillan

Usually I just leave the ticket as accepted and add the suggestion. I'm glad you pointed that we have this problem in other places... I'm a big fan of consistency.

comment:7 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:8 Changed 5 years ago by Calvin Spealman

Interestingly the base64 of the binary pickle versions is actually larger than the older text-formats, based on some rude tests i made up. If any change were made to the pickle format used in the cache, it should be to not base64 the DB cache backend and store the pickle data into proper blobs that can handle binary data.

comment:9 Changed 5 years ago by Michael Manfre

Owner: changed from nobody to Michael Manfre

comment:10 Changed 5 years ago by Michael Manfre

Owner: changed from Michael Manfre to nobody

comment:11 Changed 5 years ago by Paul McMillan

Owner: changed from nobody to Paul McMillan
Severity: NormalRelease blocker

Marking this as a release blocker and assigning to myself.

comment:12 Changed 5 years ago by Paul McMillan

Resolution: fixed
Status: newclosed

In [17136]:

Fixed #16378. Locmem now uses pickle.HIGHEST_PROTOCOL for better compatibility with other hash backends. Thanks aaugustin for the initial patch.

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