Code

Opened 3 years ago

Closed 3 years ago

#16378 closed Bug (fixed)

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

Reported by: PaulM Owned by: PaulM
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

Description

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 aaugustin 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by aaugustin

  • Easy pickings set
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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/db.py. 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 3 years ago by aaugustin

comment:2 Changed 3 years ago by rakuco

  • Cc kubo@… added

comment:3 Changed 3 years ago by adamnelson

  • Has patch unset
  • Triage Stage changed from Accepted to Unreviewed
  • Version changed from 1.3 to SVN

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 3 years ago by PaulM

  • Triage Stage changed from Unreviewed to Accepted

@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 3 years ago by adamnelson

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 3 years ago by PaulM

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 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:8 Changed 3 years ago by calvinspealman

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 3 years ago by manfre

  • Owner changed from nobody to manfre

comment:10 Changed 3 years ago by manfre

  • Owner changed from manfre to nobody

comment:11 Changed 3 years ago by PaulM

  • Owner changed from nobody to PaulM
  • Severity changed from Normal to Release blocker

Marking this as a release blocker and assigning to myself.

comment:12 Changed 3 years ago by PaulM

  • Resolution set to fixed
  • Status changed from new to closed

In [17136]:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.