classification
Title: pickle.py's load_appends should call append() on objects other than lists
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.3, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: alexandre.vassalotti Nosy List: alexandre.vassalotti, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-04-13 18:04 by alexandre.vassalotti, last changed 2017-01-26 10:52 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
loads_appends.patch alexandre.vassalotti, 2013-04-13 18:04
fix_loads_appends.patch alexandre.vassalotti, 2013-04-14 06:55
Messages (10)
msg186774 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2013-04-13 18:04
In pickle.py, load_appends is currently defined as

    def load_appends(self):
        stack = self.stack
        mark = self.marker()
        list = stack[mark - 1]
        list.extend(stack[mark + 1:])
        del stack[mark:]

However, according to the spec of APPENDS, it should actually be:

    def load_appends(self):
        obj = stack[mark - 1]
        items = stack[mark + 1:]
        if isinstance(obj, list):
            obj.extend(items)
        else:
            for item in items:
                obj.append(item)

This will match with the current behaviour of _pickle.
msg186799 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-13 18:56
Your patch is bogus (duplicated load_appends definition).
Also, I think you want to add a test if you want to avoid further regressions.
msg186800 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-13 19:07
Yes, test is needed. Current tests don't cover this case.

I suggest to move obj.append out of the tight loop, as in C implementation.

            append = obj.append
            for item in items:
                append(item)
msg186885 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2013-04-14 06:55
Alright alright! Here's a less bogus patch. :)
msg187454 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-20 20:28
New changeset 37139694aed0 by Alexandre Vassalotti in branch '3.3':
Isuse #17720: Fix APPENDS handling in the Python implementation of Unpickler
http://hg.python.org/cpython/rev/37139694aed0
msg261188 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-03-04 14:33
Why this patch was not applied to 2.7? What is the spec of APPENDS?
msg286028 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-22 20:15
From PEP 7:

    listitems    Optional, and new in this PEP.
                 If this is not None, it should be an iterator (not a
                 sequence!) yielding successive list items.  These list
                 items will be pickled, and appended to the object using
                 either obj.append(item) or obj.extend(list_of_items).
                 This is primarily used for list subclasses, but may
                 be used by other classes as long as they have append()
                 and extend() methods with the appropriate signature.
                 (Whether append() or extend() is used depends on which
                 pickle protocol version is used as well as the number
                 of items to append, so both must be supported.)

Both append() or extend() must be supported, therefore old code was correct. C implementation can be optimized by using extend().
msg286219 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2017-01-24 21:46
I don't recall where I picked up the referred spec. Looking at PEP 307 and pickle docs [1], it does look like that objects which provides the listitems field of the reduce tuple should support extend() too.

It might best to try both in the implementation to avoid breaking existing code that relies on the previous behavior.

[1]: https://docs.python.org/3/library/pickle.html#object.__reduce__
msg286238 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-25 09:56
Thanks. Opened issue29368 for this.
msg286305 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-26 10:52
> From PEP 7:

Sorry, I meant PEP 307 of course.
History
Date User Action Args
2017-01-26 10:52:58serhiy.storchakasetmessages: + msg286305
2017-01-25 09:56:40serhiy.storchakasetstatus: open -> closed

messages: + msg286238
2017-01-24 21:46:42alexandre.vassalottisetmessages: + msg286219
2017-01-22 20:15:19serhiy.storchakasetstatus: closed -> open

messages: + msg286028
2016-03-04 14:33:15serhiy.storchakasetmessages: + msg261188
2013-04-20 20:30:03alexandre.vassalottisetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2013-04-20 20:28:11python-devsetnosy: + python-dev
messages: + msg187454
2013-04-14 06:55:19alexandre.vassalottisetfiles: + fix_loads_appends.patch

messages: + msg186885
stage: needs patch -> patch review
2013-04-13 19:07:10serhiy.storchakasetmessages: + msg186800
2013-04-13 18:56:53pitrousetmessages: + msg186799
2013-04-13 18:04:49alexandre.vassalotticreate