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:

1) Classes with __slot__. In this case an attempt to pickle will raise an exception with explaining message.

2) 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 issue21975, issue22955.

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.

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.

Here is sample patch which adds two restrictions. Default reduce method for protocol >= 2 will forbid pickling objects:

1) When tp_new == NULL.

2) Builtins without any of pickle-related methods: __getnewargs_ex__, __getnewargs__ or __getstate__.

Are there any other ideas?

How many cases does the patch catch?

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

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.

Ping.

See also issue14350 and issue24900. 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.

Could anyone please make a review?

New changeset c8841db9433d by Serhiy Storchaka in branch '3.4':
Issue #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 #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 #22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/a2f574896f49

New changeset 94664fb4354e by Serhiy Storchaka in branch '2.7':
Issue #22995: Backported additional tests for non-pickleable types.
https://hg.python.org/cpython/rev/94664fb4354e

New changeset 2b950eba9792 by Serhiy Storchaka in branch '2.7':
Issue #22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/2b950eba9792

Was committed the restriction for tp_new == NULL.

New patch uses more complex condition for types that doesn't define any reduce-related methods. This case covers _csv.Dialect and memoryview (and buffer in 2.7, issue8323).

New patch more accurate detects objects with a state. It compares object's size with calculated minimal size, counting __dict__, __weakref__ and slots.

New changeset 0cd2de69fb66 by Serhiy Storchaka in branch '3.5':
Issue #22995: Instances of extension types with a state that aren't
https://hg.python.org/cpython/rev/0cd2de69fb66

New changeset b8d108a2a38e by Serhiy Storchaka in branch 'default':
Issue #22995: Instances of extension types with a state that aren't
https://hg.python.org/cpython/rev/b8d108a2a38e

New changeset 92172d7372dd by Serhiy Storchaka in branch '2.7':
Issue #22995: Instances of extension types with a state that aren't
https://hg.python.org/cpython/rev/92172d7372dd

That's all with this issue.

Analyzing the signature of __new__ is not such easy and perhaps will be done in separate issue.

I'm reopening this issue because we're starting to see a bunch of regressions in Debian and Ubuntu we think is caused by the changes here.  We've definitely started seeing them with 3.5.1, where packages that use Cython built just fine in 3.5.0 but now fail with TypeError in 3.5.1.  I haven't been able to completely debug the problem yet, but here's what I know.

Here's a build log for s3ql that shows the failure: http://paste.debian.net/361351/

Here's a bug report showing a build failure for kivy, leading to the same problem: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=810139

Nothing immediately jumps out at me as problematic with the NameAssignment class, which afaict is defined in Cython/Compiler/FlowControl.py as thus:

class NameAssignment(object):
    def __init__(self, lhs, rhs, entry):
        if lhs.cf_state is None:
            lhs.cf_state = set()
        self.lhs = lhs
        self.rhs = rhs
        self.entry = entry
        self.pos = lhs.pos
        self.refs = set()
        self.is_arg = False
        self.is_deletion = False
        self.inferred_type = None

    def __repr__(self):
        return '%s(entry=%r)' % (self.__class__.__name__, self.entry)

    def infer_type(self):
        self.inferred_type = self.rhs.infer_type(self.entry.scope)
        return self.inferred_type

    def type_dependencies(self):
        return self.rhs.type_dependencies(self.entry.scope)

    @property
    def type(self):
        if not self.entry.type.is_unspecified:
            return self.entry.type
        return self.inferred_type


I suppose it's possible that something outside the class is poking an unpicklable attribute into the instance, but wouldn't that show up as a pickling error of a different class?  

The TypeError is being raised in deepcopy() in copy.py, where it is getting x's __reduce_ex__ and then calling reductor(4) (see ~line 174).  If you catch the TypeError on that line and set a breakpoint, then call `copy(x)` you see the TypeError.

I tried calling copy.deepcopy() on all the attributes of the NameAssignment instance I could find, and they all copied just fine.

I'm going to try to set up a gdb to figure out exactly which of the new picklability tests is failing, but I definitely think we have a regression here.  I suppose it's also possible that it's a legitimate bug in Cython, but then how did it ever work before?

What return __reduce_ex__(4) for the NameAssignment instance in 3.5.0?

On Jan 06, 2016, at 10:13 PM, Serhiy Storchaka wrote:

>What return __reduce_ex__(4) for the NameAssignment instance in 3.5.0?

I'm not sure yet (it takes some time to set up the right environment to test
this), but I do know which TypeError is getting triggered.  In typeobject.c,
it's this stanza in _PyObject_GetState():

        assert(slotnames == Py_None || PyList_Check(slotnames));
        if (required) {
            Py_ssize_t basicsize = PyBaseObject_Type.tp_basicsize;
            if (obj->ob_type->tp_dictoffset)
                basicsize += sizeof(PyObject *);
            if (obj->ob_type->tp_weaklistoffset)
                basicsize += sizeof(PyObject *);
            if (slotnames != Py_None)
                basicsize += sizeof(PyObject *) * Py_SIZE(slotnames);
            if (obj->ob_type->tp_basicsize > basicsize) {
                Py_DECREF(slotnames);
                Py_DECREF(state);
                PyErr_Format(PyExc_TypeError,
                             "can't pickle %.200s objects",
                             Py_TYPE(obj)->tp_name);
                return NULL;
            }
        }

I don't yet know why the basicsize isn't matching up.

I can understand why. AFAIK Cython creates a class that stores attributes not in __dict__, but packed in a structure, as for __slots__. This increases the basicstate. But if it doesn't provide nor __dict__, nor __slotnames__, nor any pickle-related method, the default implementation object.__reduce_ex__ returned a value that didn't contain the full state of the object. Therefore the object couldn't be correctly unpickled, copied, or deepcopied. If this is true, there is a bug in Cython. But if deepcopying worked for Cython objects, we need to find how object's state is retrieved and take this into account in our code.

If this is Cython bug, may be we have to change an exception to a warning in maintained releases.

Your explanation still doesn't entirely make sense to me yet because the crash is happening in what seems to be a Cython implementation class, not a generated class based on a .pyx in some other project using Cython.

I tried to search the Cython Trac but either my Trac-fu sucks or nobody has yet reported compatibility problems with Python 3.5.1 to upstream.  I don't see any other relevant bug reports, and Cython's git head passes its test suite with Python 3.5.1 (other than the asyncio_generators test which is probably unrelated and has already been reported to the Cython mailing list).

It's always possible of course that the Python change is triggering a bug not caught by the Cython test suite, but something as fundamental as pickling would be surprising if not tested.

Turning the error into a warning does have a lot of appeal for 3.5 maintenance.  It could be kept as an error in 3.6.  At the least, can the exception text include more information about *why* the pickling failure occurs. (Note that the exact error message can be raised in multiple places, so I had to build a custom 3.5.1 to distinguish between them; I don't have gdb set up in the build environment yet.)

It's still worth better understanding why this change is causing the bugs in projects using Cython, and reviewing what happens with Python 3.5.0.  For completeness, I also want to verify that there isn't some problem specifically related to Ubuntu's version of Python 3.5.1, although the tests against Cython's git head use the same version.

I'm not well known with Cython. Can it replace Python class with extension class on fly? There is a declaration of the NameAssignment class in Cython/Compiler/FlowControl.pxd. We need Cython expert.

You asked what reductor(4) returns in Python 3.5.0:

> /usr/lib/python3.5/copy.py(176)deepcopy()
-> rv = reductor(4)
(Pdb) reductor(4)
(<function __newobj__ at 0x7f7a5f77e158>, (<class 'Cython.Compiler.FlowControl.NameAssignment'>,), None, None, None)

And this is completely reasonable:

(Pdb) p rv
(<function __newobj__ at 0x7f7a5f77e158>, (<class 'Cython.Compiler.FlowControl.NameAssignment'>,), None, None, None)
(Pdb) p rv[0](*rv[1])
NameAssignment(entry=None)

I'm doing another debug build of Python to get more information about which extra bit of basicsize is getting added to trigger the TypeError.

I added some debugging to the if(required) clause.  This is interesting:

basicsize 16
slotnames 0 basicsize 16
tp_basicsize 80 basicsize 16

tp_basicsize comes in at 16 bytes.  tp_dictoffset and tp_weaklistoffset must both be 0, and while slotnames is not Py_None, it must be adding 0 to the basicsize.  Since sizeof(PyObject *) can't be 0, it must mean that Py_SIZE(slotnames) is 0 (since they are multiplied).

But as you can see tp_basicsize is 80.  So essentially PyBaseObject_Type.tp_basicsize is 16 but obj->ob_type->tp_basicsize is 80, and that triggers the traceback.

So yeah, I guess Cython is doing something magical to boost the instance's type's basicsize, but I can't tell what from the Python prompt.  It also clearly doesn't affect the un/picklability of the instance, so I think the entire check is probably misguided.

It should almost definitely not raise a TypeError.  Either disable the entire check, or at most make it a warning.

I've verified that backing out the typeobject.c change allows kivy and pysam to build.  s3ql still fails but for other reasons, though its build does get past the cython failure.  This clinches it for me.  I am going to remove the typeobject.c clause from 2.7, 3.5, and 3.6.

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.

On Jan 11, 2016, at 05:50 PM, Serhiy Storchaka wrote:

>
>Therefore my guess was true. There is a bug in Cython. It creates classes
>that can't be correctly unpickled or copied.

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.

>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.

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.

New changeset e79eddcdff63 by Barry Warsaw in branch '3.5':
Issue #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 #22995: [UPDATE] Comment out the one of the pickleability tests in
https://hg.python.org/cpython/rev/927fd0e14d49

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:

>>> import csv, pickle
>>> d = csv.get_dialect('excel-tab')
>>> d2 = pickle.loads(pickle.dumps(d))
>>> d.delimiter, d.doublequote, d.escapechar, d.lineterminator, d.quotechar, d.quoting, d.skipinitialspace, d.strict
('\t', 1, None, '\r\n', '"', 0, 0, 0)
>>> d2.delimiter, d2.doublequote, d2.escapechar, d2.lineterminator, d2.quotechar, d2.quoting, d2.skipinitialspace, d2.strict
(',', 1, None, '\r\n', '"', 0, 0, 0)

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.

On Jan 11, 2016, at 07:20 PM, Serhiy Storchaka wrote:

>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.

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!

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.

New changeset 9d3ac16b78dc by Barry Warsaw in branch '2.7':
Issue #22995: [UPDATE] Comment out the one of the pickleability tests in
https://hg.python.org/cpython/rev/9d3ac16b78dc

Thank you for reverting the change in 2.7 and 3.5 Barry.

> can you please file a bug with upstream Cython?

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.

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.

Thanks Stefan and Serhiy!  I think this issue is now resolved.

For reference, the bug in Cython is fixed here:

https://github.com/cython/cython/commit/ececb3e9473f6aaa65f29467921594c316ec2f06

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?

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.