Repository navigation
Restrict default pickleability #67184
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-01-12.14:25:27.091>
created_at = <Date 2014-12-04.13:16:43.440>
labels = ['interpreter-core', 'type-bug']
title = 'Restrict default pickleability'
updated_at = <Date 2016-01-16.12:44:45.844>
user = 'https://github.com/serhiy-storchaka'bugs.python.org fields:
activity = <Date 2016-01-16.12:44:45.844>
actor = 'scoder'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-01-12.14:25:27.091>
closer = 'barry'
components = ['Interpreter Core']
creation = <Date 2014-12-04.13:16:43.440>
creator = 'serhiy.storchaka'
dependencies = []
files = ['37494', '37739', '40373', '41024', '41363', '41582']
hgrepos = []
issue_num = 22995
keywords = ['patch']
message_count = 40.0
messages = ['232135', '232160', '232887', '234125', '234131', '234175', '242962', '249901', '251723', '254525', '254526', '254528', '254562', '256733', '257006', '257234', '257235', '257646', '257650', '257667', '257686', '257706', '257719', '257720', '257724', '257725', '257979', '257993', '257998', '257999', '258000', '258002', '258003', '258004', '258099', '258101', '258105', '258377', '258385', '258386']
nosy_count = 10.0
nosy_names = ['barry', 'pitrou', 'scoder', 'alexandre.vassalotti', 'nadeem.vawda', 'Arfrever', 'kitterma', 'python-dev', 'serhiy.storchaka', 'josh.r']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22995'
versions = ['Python 2.7', 'Python 3.5']
Activity
serhiy-storchaka commentedon Dec 4, 2014
When no efforts were spent to support pickling of instances of Python class, in most cases the class will be pickleable. Default implementation just saves all attributes and this works if all attributes are pickleable. Exceptional special cases:
Classes with __slot__. In this case an attempt to pickle will raise an exception with explaining message.
Classes with __new__ wish mandatory parameters. In this case the pickling will be silently "successful", but actually data will be not saved and unpickling will fail. See for example bpo-21975, bpo-22955.
But when the class is implemented in C and doesn't expose its attribute in __dict__, in most cases it is variant (2) -- silent incorrect pickling.
I think we should prevent incorrect pickling by default. It is better to fail loudly. Default reduce could inspect the __new__ method and raise an exception if it has non-optional parameters and the class doesn't implement __getnewargs_ex__ or __getnewargs__ methods. And fail on all classes implemented in C if they don't implement any pickle-related methods.
[-]Restrict default picleability[/-][+]Restrict default pickleability[/+]MojoVampire commentedon Dec 4, 2014
Minor note: Python classes with __slots__ pickle and unpickle just fine under protocol 2 and higher. Only protocols 0 and 1 have this problem, and they are no longer used by default on Py3, and not necessary to communicate with Py2, where protocol 2 is supported and generally more efficient.
I understand that you're not suggesting any changes to address issue #1, just wanted to mention this.
serhiy-storchaka commentedon Dec 18, 2014
Here is sample patch which adds two restrictions. Default reduce method for protocol >= 2 will forbid pickling objects:
When tp_new == NULL.
Builtins without any of pickle-related methods: __getnewargs_ex__, __getnewargs__ or __getstate__.
Are there any other ideas?
pitrou commentedon Jan 16, 2015
How many cases does the patch catch?
serhiy-storchaka commentedon Jan 16, 2015
All public classes not designed for pickling explicitly. I tested only operator.methodcaller, mmap.mmap, sqlite3 classes (Connect, Cursor, Row), _socket.socket, select.epoll, _csv.Dialect, but should be more. Instances of these classes can be "pickled", but unpickling either raise an exception (usually TypeError), or returns default or underinitialized object.
For other classes the patch changes raised exception type. E.g. pickling zlib.compressobj() raised
_pickle.PicklingError: Can't pickle <class 'zlib.Compress'>: attribute lookup Compress on zlib failed
and with the patch it raises
TypeError: can't pickle zlib.Compress objects
serhiy-storchaka commentedon Jan 17, 2015
It is hard to write good tests, because every example is temporary, after adding support of pickling it will be not valid.
Here is extended patch. It now handles Python subclasses of builtin classes (except list and dict which are pickleable via iterators) such as socket.socket. Removed several guarding __getstate__() methods. Added few new tests.
serhiy-storchaka commentedon May 12, 2015
Ping.
serhiy-storchaka commentedon Sep 5, 2015
See also bpo-14350 and bpo-24900. Both will be solved by disabling pickling when tp_new is NULL (a part of proposed patch). So that I think the patch should be applied to all releases.
serhiy-storchaka commentedon Sep 27, 2015
Could anyone please make a review?
python-dev commentedon Nov 12, 2015
New changeset c8841db9433d by Serhiy Storchaka in branch '3.4':
Issue bpo-22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/c8841db9433d
New changeset 4c05e7c195ac by Serhiy Storchaka in branch '3.5':
Issue bpo-22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/4c05e7c195ac
New changeset a2f574896f49 by Serhiy Storchaka in branch 'default':
Issue bpo-22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/a2f574896f49
20 remaining items
serhiy-storchaka commentedon Jan 11, 2016
Therefore my guess was true. There is a bug in Cython. It creates classes that can't be correctly unpickled or copied.
The question is wherever we should replace TypeError with a warning in maintained releases, or remove this check at all? Here is a patch against 3.5 that replaces TypeError with a warning.
warsaw commentedon Jan 11, 2016
On Jan 11, 2016, at 05:50 PM, Serhiy Storchaka wrote:
I don't think that's necessarily true. Clearly the classes *can* be copied
and unpickled because they *are*, otherwise Cython builds would be failing for
a long time now, and I can find no evidence of that either in Debian/Ubuntu
packages which use Cython nor in upstream bug reports for Cython or the
affected packages. It's just as likely that the assumptions of the
typeobject.c code are incorrect.
I am final testing a patch to typeobject.c that ifdefs that test out in 2.7,
3.5 and 3.6. I think given the observed regressions, this is the right way to
go for now. If you can figure out more detail about what's going on and can
adjust the change to avoid the regressions, then this code can be unifdef'd
away and fixed.
python-dev commentedon Jan 11, 2016
New changeset e79eddcdff63 by Barry Warsaw in branch '3.5':
Issue bpo-22995: [UPDATE] Comment out the one of the pickleability tests in
https://hg.python.org/cpython/rev/e79eddcdff63
New changeset 927fd0e14d49 by Barry Warsaw in branch 'default':
Issue bpo-22995: [UPDATE] Comment out the one of the pickleability tests in
https://hg.python.org/cpython/rev/927fd0e14d49
serhiy-storchaka commentedon Jan 11, 2016
The classes can't be *correctly* copied and unpickled because the pickle data doesn't contain object's state. "Copied" and "unpickled" objects are not equal to original objects, they are non-initialized instances. You can test attributes of copied NameAssignment instance to ensure that they are not restored. May be copied values are not used in Cython building, except rare exclusive cases when using them fails.
Example in the stdlib is csv.Dialect objects:
You just silently get wrong result. Since it by accident matches the instance of most used 'excel' dialect, this error is left unnoticed.
I think we have to left this restriction in 3.6 (and Cython should fix its bug by providing either __getnewargs__/ __getnewargs_ex__, __getstate__ or __reduce__/reduce_ex). But in 2.7 and 3.5 we should allow current Cython to work (even obtaining wrong result) by removing the check at all or by making it emit only a warning.
warsaw commentedon Jan 11, 2016
On Jan 11, 2016, at 07:20 PM, Serhiy Storchaka wrote:
I'm okay with reverting the change in 2.7 and 3.5 since those are clearly
regressions (whether or not Cython unpickles/copies correctly build failures
are regressions).
I'm also okay with leaving the restriction in 3.6, at least for now. We can
let Cython upstream complain if they disagree with the location of the bug.
I will update the various branches. Thanks for the good discussion!
warsaw commentedon Jan 11, 2016
BTW Serhiy, can you please file a bug with upstream Cython? I think you're the right person to do it since you added this restriction. With reverting this in 2.7 and 3.5 it's not urgent, but we don't want to have to reopen this when 3.6 is out.
python-dev commentedon Jan 11, 2016
New changeset 9d3ac16b78dc by Barry Warsaw in branch '2.7':
Issue bpo-22995: [UPDATE] Comment out the one of the pickleability tests in
https://hg.python.org/cpython/rev/9d3ac16b78dc
serhiy-storchaka commentedon Jan 12, 2016
Thank you for reverting the change in 2.7 and 3.5 Barry.
It is not so easy. Cython bugtracker doesn't allow free registration or anonymous reporting. I have submitted a report to Cython core developer mailing list, but it still waits for moderating. AFAIK Stefan is active Cython core developer, may be his could raise this issue for Cython core team.
scoder commentedon Jan 12, 2016
Thanks for calling me in. I can confirm that the stack trace in https://paste.debian.net/361351/ points at a bug in Cython's own implementation (not the code it generates). It's deep-copying objects that don't support it (which doesn't normally have an impact because they aren't actually used). I'll try to fix that.
Given this specific case, i.e. deep-copying objects to death "by accident", however shows that this change is clearly a) helpful and b) harmful. It's going to help people write safer code, but it will also break existing working code that can't normally fail as it is and that people might not really want to (or even cannot) touch any more. Not really something for a backport, IMHO.
warsaw commentedon Jan 12, 2016
Thanks Stefan and Serhiy! I think this issue is now resolved.
scoder commentedon Jan 16, 2016
For reference, the bug in Cython is fixed here:
cython/cython@ececb3e
serhiy-storchaka commentedon Jan 16, 2016
Nice.
Did you considered the possibility to implement the pickling compatible with non-accelerated Python classes with __dict__ for every Cython class? I.e. __getstate__ that returns a dict that maps field names to values and __setstate__ that unpacks such dict?
scoder commentedon Jan 16, 2016
In fact, I did consider it and I'd love to, but it's not something to do lightly because the semantics would be a bit fuzzy and unsafe. Basically, extension types that have only Python object compatible attributes could automatically pickle, but as soon as you add, say, a C pointer attribute, it would fail to pickle at runtime, maybe even without an error (as in the case at hand). And what do you think, how many users write unit tests for pickling all of their extension types?
Imagine the case that Cython adds automatic pickling support for a base class and a user tries to implement pickling in a subclass, but using a different set of pickle support methods. It might then happen that the user implementation is silently ignored by the pickle implementation because it prefers the methods in the base class. The user might not even notice this because it (mostly) seems to work. And now think of the case where this user code exists already and Cython starts to enable the base class pickle support with a new release and overrides the user implementation. That might lead to seriously difficult to find bugs.
It would be great to make this support the default rather than requiring user interaction (say, a class decorator or compiler directive), but the subtle code breakage cases above sadly suggest otherwise...
Anyway, this needs more thinking and this ticket isn't the right place to do that.