classification
Title: Confusing exception from cPickle on reduce failure
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: python-dev, raoul_gough_baml, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-12-09 18:11 by raoul_gough_baml, last changed 2016-12-15 12:09 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
cpickle_reduce_failure.py raoul_gough_baml, 2016-12-09 18:11
load_classic_instance_error-2.7.patch serhiy.storchaka, 2016-12-10 22:13 review
Messages (5)
msg282795 - (view) Author: Raoul Gough (raoul_gough_baml) Date: 2016-12-09 18:11
Description
===========

Running the attached example program with Python 2.7.12 produces the output below. The demo deliberately raises a user-defined exception during the unpickling process, but the problem is that this exception does not propagate out of the unpickle call. Instead, it gets converted into a TypeError which is confusing and does not help identify the original problem.

INFO:root:Creating BadReduce object
INFO:root:Pickling
INFO:root:Unpickling
INFO:root:Raising exception "BadReduce init failed"
Traceback (most recent call last):
  File "cpickle_reduce_failure.py", line 48, in <module>
    main()
  File "cpickle_reduce_failure.py", line 41, in main
    pickler.loads(s1)
  File "cpickle_reduce_failure.py", line 27, in __init__
    raise exception
TypeError: __init__() takes exactly 2 arguments (4 given)

If you change the demo to use the Python pickle module, i.e. "import pickle as pickler", it produces the expected output below:

INFO:root:Creating BadReduce object
INFO:root:Pickling
INFO:root:Unpickling
INFO:root:Raising exception "BadReduce init failed"
INFO:root:Got MyException "BadReduce init failed"
INFO:root:Done

Analysis
========

The following code in Modules/cPickle.c in the function Instance_New (around https://github.com/python/cpython/blob/2.7/Modules/cPickle.c#L3917) does a PyErr_Restore with the exception type MyException, as raised from BadReduce.__init__, but it replaces the exception value with a tuple (original_exception, cls, args):

        PyObject *tp, *v, *tb, *tmp_value;

        PyErr_Fetch(&tp, &v, &tb);
        tmp_value = v;
        /* NULL occurs when there was a KeyboardInterrupt */
        if (tmp_value == NULL)
            tmp_value = Py_None;
        if ((r = PyTuple_Pack(3, tmp_value, cls, args))) {
            Py_XDECREF(v);
            v=r;
        }
        PyErr_Restore(tp,v,tb);

Later, PyErr_NormalizeException attempts to convert the exception value (the tuple) to the original exception type. This fails because MyException.__init__() can't handle the multiple arguments. This is what produces the TypeError "__init__() takes exactly 2 arguments (4 given)"


Proposed Fix
============

I think it would be better if Instance_New did the PyErr_NormalizeException itself instead of allowing it to be done lazily. If the normalize works, i.e. the exception type accepts the extra arguments, the behaviour would be almost unchanged - the only difference being the PyErr_NormalizeException happens earlier. However, if the normalize fails, Instance_New can restore the original exception unchanged. That means that we lose the cls, args information in this case, but not the original exception.
msg282879 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-10 22:13
I think it is better to remove this code.
msg283272 - (view) Author: Raoul Gough (raoul_gough_baml) Date: 2016-12-15 10:00
Hi Serhiy, thanks very much for looking at this. My only concern with removing the code completely is that it does work (presumably as intended) with at least some of the standard exception types. For example, if you change the demo to raise and catch a RuntimeError instead of MyException, the output looks like this:

INFO:root:Creating BadReduce object
INFO:root:Pickling
INFO:root:Unpickling
INFO:root:Raising exception "BadReduce init failed"
INFO:root:Got RuntimeError "(RuntimeError('BadReduce init failed',), <class '__main__.BadReduce'>, (1123,))"
INFO:root:Done

Where the caught RuntimeError contains the original RuntimeError and some additional information from cPickle.

I also checked that it correctly propagates KeyboardInterrupt by testing manually with a sleep:

INFO:root:Creating BadReduce object
INFO:root:Pickling
INFO:root:Unpickling
INFO:root:Sleeping a while - send keyboard interrupt to test
  C-c C-cTraceback (most recent call last):
  File "/home/zk8ms03/python/cpickle_reduce_failure.py", line 49, in <module>
    main()
  File "/home/zk8ms03/python/cpickle_reduce_failure.py", line 42, in main
    pickler.loads(s1)
  File "/home/zk8ms03/python/cpickle_reduce_failure.py", line 28, in __init__
    time.sleep(5.0)
KeyboardInterrupt: (None, <class '__main__.BadReduce'>, (1123,))


I'm not sure how likely it is that anyone depends on the extra information that cPickle adds in these cases, so I'll leave it up to others to decide what's best.
msg283278 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-15 10:33
Perhaps it was added for debugging. Exceptions that don't define own __new__ and __init__ inherit them from BaseException. BaseException constructor saves positional arguments as the args attribute. But not all standard exceptions accept arbitrary number of positional arguments. For example UnicodeEncodeError and UnicodeDecodeError don't work.
msg283281 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-15 10:56
New changeset eb830f1511ef by Serhiy Storchaka in branch '2.7':
Issue #28925: cPickle now correctly propagates errors when unpickle instances
https://hg.python.org/cpython/rev/eb830f1511ef
History
Date User Action Args
2016-12-15 12:09:30serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-12-15 10:56:00python-devsetnosy: + python-dev
messages: + msg283281
2016-12-15 10:33:46serhiy.storchakasetmessages: + msg283278
2016-12-15 10:00:56raoul_gough_bamlsetmessages: + msg283272
2016-12-10 22:13:47serhiy.storchakasetfiles: + load_classic_instance_error-2.7.patch
keywords: + patch
messages: + msg282879

stage: needs patch -> patch review
2016-12-09 20:49:44serhiy.storchakasetassignee: serhiy.storchaka

nosy: + serhiy.storchaka
stage: needs patch
2016-12-09 18:11:16raoul_gough_bamlcreate