classification
Title: Fix exception pickling: Move initial args assignment to BaseException.__new__
Type: behavior Stage: patch review
Components: Interpreter Core Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: lukasz.langa Nosy List: alexandre.vassalotti, belopolsky, bpb, brett.cannon, ehuss, facundobatista, fmitha, georg.brandl, gvanrossum, haypo, jafo, jarpa, jason.coombs, kylev, loewis, lukasz.langa, nnorwitz, pitrou, taleinat, tseaver, zbysz, zseil
Priority: critical Keywords: patch

Created on 2007-04-01 13:46 by zseil, last changed 2012-03-11 10:17 by eric.araujo.

Files
File name Uploaded Description Edit
exc_args.diff zseil, 2007-04-01 13:46 patch against trunk revision 54643 review
exc_args_25.diff zseil, 2007-04-01 13:47 patch against release25-maint branch revision 54643 review
test_exception_pickle.py zseil, 2007-04-01 21:50 failing test
exception-pickling.diff georg.brandl, 2007-08-11 09:06 patch by Georg review
exception-pickling.diff georg.brandl, 2007-08-11 09:06 patch by Georg review
exception_pickling_25.diff zseil, 2007-08-12 11:23 __getstate__() patch for the 2.5 branch review
exception_pickling_26.diff zseil, 2007-08-12 11:24 __getstate__() patch for the 2.6 branch review
exception_pickling_26.diff jarpa, 2008-01-17 22:13 fixed rejects and fuzz on the current svn version
issue1692335-tests.patch tseaver, 2010-05-31 21:13 Patch demonstrating the bug
Messages (38)
msg52348 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-04-01 13:46
Pickling exceptions fails when an Exception class
requires an argument in the constructor, but doesn't
call its base class' constructor.  See this mail
for details:

http://mail.python.org/pipermail/python-dev/2007-April/072416.html

This patch simply moves initial args assignment to
BaseException.__new__. This should fix most of the
problems, because it is very unlikely that an
exception overwrites the __new__ method; exceptions
used to be old style classes, which don't support
the __new__ special method.

The args attribute is still overwritten in all the
__init__ methods, so there shouldn't be any backward
compatibility problems.

msg52349 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-04-01 13:47
File Added: exc_args_25.diff
msg52350 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-04-01 21:50
I'm attaching a test that Eric Huss sent in private
mail.

The test fails, because old exception pickles don't
have args for the reconstructor, but their __init__()
gets called anyway because they are new style classes
now.

The problem is in cPickle.InstanceNew() and
pickle.Unpickler._instantiate().  Those methods behave
differently depending on the type of the object
instantiated; they avoid the __init__() call when type
is an old style class.

There is nothing that can be done in the exception
classes to fix this issue; the fix would need to be
in the pickle and cPickle module.
File Added: test_exception_pickle.py
msg52351 - (view) Author: Eric Huss (ehuss) Date: 2007-06-15 00:34
I have stumbled across another scenario where unpickling fails.  If your exception takes arguments, but you call Exception.__init__ with a different number of arguments, it will fail.  As in:

class D(Exception):
    def __init__(self, foo):
        self.foo = foo
        Exception.__init__(self)
msg52352 - (view) Author: Eric Huss (ehuss) Date: 2007-06-27 18:45
Added patch #1744398 as an alternate solution.
msg52353 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-08-11 09:06
Attaching a new patch that fixes the 

class D(Exception):
    def __init__(self, foo):
        self.foo = foo
        Exception.__init__(self)

scenario by keeping the original args to __new__ as an exception attribute.
File Added: exception-pickling.diff
msg52355 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-08-11 18:27
self->args needs to be initialized to NULL because if the alloc of args failed, self->args would be uninitialized and deallocated.  (I realize the alloc of an empty tuple will realistically never fail currently.)

I'm not sure if this could cause any new problems because of the behavior change, but the code itself looked fine to me.  Hopefully someone with more knowledge of exceptions could take a look at the idea.
msg52356 - (view) Author: Eric Huss (ehuss) Date: 2007-08-12 01:33
Georg's new patch looks good to me, it seems to pass all the scenarios that I know of.  You can close out my patch if you check this in.  Thanks for looking into this!
msg52357 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-08-12 08:00
The only question left is what to do if something reassigns exceptioninstance.args.
msg52359 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-08-12 11:23
There is also the problem that Jim Fulton mentioned
in bug #1742889; if the user modifies the 'message'
attribute of an exception (or any other attribute
that is not stored in the __dict__), it won't get
serialized on pickling.

Attaching a new patch that fixes this by using the
__getstate__() pickling hook.  It uses tp_members
and tp_getset members of the type object to find
all the attributes that need to be serialized.

The 2.5 patch also contains a fix for migrating
old exception pickles to Python 2.5.  For that
I had to change cPickle and pickle.  It would
be nice if Jim could comment if this change is
needed, since ZODB is probably the biggest user
of these modules.

The downside is that this patch is fairly big.

File Added: exception_pickling_25.diff
msg52360 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-08-12 11:24
File Added: exception_pickling_26.diff
msg52361 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-08-12 12:13
I wouldn't care too much about .message; it was new in 2.5 and is already deprecated in 2.6.
msg52362 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2007-08-12 12:34
It's not just the message attribute; the problem is
that the current code expects that the exception
won't be modified between creation and unpickling.

For example:

>>> import pickle
>>> e = EnvironmentError()
>>> e.filename = "x"
>>> new = pickle.dumps(pickle.loads(e))
>>> print new.filename
None
>>> e = SyntaxError()
>>> e.lineno = 10
>>> new = pickle.loads(pickle.dumps(e))
>>> print new.lineno
None
msg52363 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-08-12 13:24
Yes, this is what I meant with my last comment.
msg55165 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-08-23 17:29
Raising priority.
msg55958 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2007-09-17 10:01
It's not clear to me what the next step is here, does one or more of the
other folks need to provide some input?  Georg: If you are looking for
something from someone, can you assign the ticket to them?
msg56054 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-09-20 08:47
IMO zseil's latest patch is the best one, but I need any other developer
to review and approve it.
msg60067 - (view) Author: Jaroslav Pachola (jarpa) Date: 2008-01-17 22:13
While zseil's patch for Python 2.5 works for me (on the current 2.5.1
download), svn version of Python 2.6 rejects the 2.6 patch. Attaching
fixed 2.6 patch (2 rejects, 1 fuzz fixed, patch works without complains
for me). I would be very glad if someone could review the patches and
maybe commit them.
msg60163 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2008-01-19 14:03
The last patch (exception_pickling_26.diff) applies cleanly, and all the
tests run ok.

System:
  Python 2.6a0 (trunk:60073M, Jan 19 2008, 11:41:33) 
  [GCC 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)] on linux2
msg61967 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-02-01 17:38
I'm against including this in 2.5.2. It reeks of a new feature, and the
code looks like it would risk breaking other apps.

(I'm fine with adding this to 2.6 of course.)
msg61997 - (view) Author: Jaroslav Pachola (jarpa) Date: 2008-02-02 08:07
To me it seems more like a bug fix - in Python 2.4 and older the
pickling works well and the current 2.5 behavior really breaks existing
code. That's why I plead for inclusion in 2.5.2; because this issue
prevents our company to move to 2.5.
msg61999 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-02-02 11:36
Understood.  Can you get other developers on python-dev to weigh in? 
Maybe I am over-estimating the danger.
msg62025 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-03 19:04
I tested exception_pickling_25.diff, and it may break existing code.
In 2.5.1, Exception("Hello,4).__reduce__() gives 
(<type 'exceptions.Exception'>, ('Hello', 4))

With the patch, it gives

TypeError: can't pickle Exception objects

IMO, that is an unacceptable change for a bugfix release.

Aside: please give unique file names to the patches, or remove patches
if you want to replace a previous patch.
msg70453 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-07-31 01:51
How is this coming? Can we apply this to 2.6?
msg75955 - (view) Author: Ziga Seilnacht (zseil) * (Python committer) Date: 2008-11-17 11:37
Sorry for the long silence. I think that this patch is not relevant
anymore. The code that uses exception pickling already had to be adapted
to changes in Python 2.5, so there is no need to change the pickling
again and risk breaking user code. I'll close it in a few days if nobody
objects.
msg76020 - (view) Author: Eric Huss (ehuss) Date: 2008-11-18 22:06
I'm disappointed to see this closed.  Exception pickling is still broken
in some cases in 2.6.
msg76026 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2008-11-18 22:52
OK, reopening. Can you post an example that fails today?
msg76028 - (view) Author: Eric Huss (ehuss) Date: 2008-11-18 23:07
In the attached test_exception_pickle.py file, class C and D cannot be
unpickled (raises TypeError).

class C(Exception):
    """Extension with values, args not set."""
    def __init__(self, foo):
        self.foo = foo

class D(Exception):
    """Extension with values, init called with no args."""
    def __init__(self, foo):
        self.foo = foo
        Exception.__init__(self)

There are also some other exceptions that fail to be handled properly. 
See the exception_pickling_26.diff for some unittests.  In particular, I
see SlotedNaiveException failing.
msg102995 - (view) Author: Kyle VanderBeek (kylev) Date: 2010-04-13 00:22
Ping?  Is anyone working on this?  Have Eric's concerns been addressed and can we pickle/unpickle all exceptions yet?
msg106811 - (view) Author: Tres Seaver (tseaver) Date: 2010-05-31 21:13
The attached patch adds Mark's examples to test_pickle as a failing test.
msg108954 - (view) Author: Alexander Belopolsky (belopolsky) (Python committer) Date: 2010-06-29 22:18
The case in msg76028 are probably not pointing to a bug.  If subclass' __init__ passes its args to the base class __init__ (as it probably should), pickling works: 

class E(Exception):
    """Extension with values, init called with args."""
    def __init__(self, foo):
        self.foo = foo
        Exception.__init__(self, foo)

>>> from pickle import *
>>> loads(dumps(E(1)))
E(1,)
>>> _.foo
1

What would be a use case for not setting args?  Even if there was a valid use case for it, it would be easy to simply provide custom __getinitargs__ or __reduce__ to support it.

Note that with msg76028 definitions, eval(repr(C('foo'))) also raises a TypeError, so properly supporting args hiding in exception subclasses would probably involve overriding __repr__ as well.
msg108960 - (view) Author: Eric Huss (ehuss) Date: 2010-06-30 03:04
Alexander, the use case I was involved with was an RPC system which allowed exceptions to propagate over the connection.  In this case, you do not have absolute control over which exceptions may be raised, or who wrote the code that is raising the exception.  There are many cases, even in the standard library, where people do not pass all arguments to the base exception class.  Some of these are probably mistakes, but in general pickle shouldn't fail on otherwise legitimate objects.

There are other cases where passing all arguments up is not wanted, or at least cumbersome.  If you have multiple levels of inheritance, and subclasses add additional arguments to the init, they wouldn't have a way to include those arguments to the base class unless all classes were written with *args in the init function, which many people would not know or remember to do.
msg117139 - (view) Author: Jason R. Coombs (jason.coombs) * (Python committer) Date: 2010-09-22 13:39
In msg108954, I believe belopolsky is mistaken in stating that "it would be easy to simply provide custom __getinitargs__ or __reduce__ to support it". It appears __getinitargs__ does not work on Python 2.5 or Python 2.7. Exceptions of the following class still raise a TypeError on unpickling:

class D(Exception):
    """Extension with values, init called with no args."""
    def __init__(self, foo):
        self.foo = foo
        Exception.__init__(self)

    def __getinitargs__(self):
        return self.foo,

Using __reduce__ does seem to work. I suspect this is because Exceptions are extension types.

I think the fundamental problem is that pickling exceptions does not follow the principle of least surprise. In particular:

 - Other built-in objects (dicts, lists, etc) don't require special handling (replace Exception with dict in the above example and it works).
 - It's not obvious how to write an Exception subclass that takes additional arguments and make it pickleable.
 - Neither the pickle documentation nor the Exception documentation describe how pickling is implemented in Exceptions.

Eric has provided some good use cases. Furthermore, it seems counter-intuitive to me to pass a subclass' custom arguments to the parent class. Indeed, even the official tutorial defines exception classes that are unpickleable (http://docs.python.org/tutorial/errors.html#tut-userexceptions).

If the use case is obvious enough that it shows up in the hello world tutorial, I don't think there should be any argument that it's not a common use case.

At the very least, there should be a section in the pickle documentation or Exception documentation describing how one should make a pickleable subclass. What would be better is for Exceptions to behave like most other classes when being pickled.
msg117145 - (view) Author: Jason R. Coombs (jason.coombs) * (Python committer) Date: 2010-09-22 14:58
After some further reading, I found that PEP-352 (http://www.python.org/dev/peps/pep-0352/) explicitly states that "Including programmatic information (e.g., an error code number) should be stored as a separate attribute in a subclass [and not in the args attribute]." The parameter to Exception.__init__ should always be a single, human-readable string. The default pickler doesn't handle this officially-prescribed use-case without overriding __reduce__.

class F(Exception):
    def __init__(self, message, code):
        Exception.__init__(self, message)
        self.code = code

Of course, as belopolsky observed, __repr__ must also be overridden.
msg117149 - (view) Author: Alexander Belopolsky (belopolsky) (Python committer) Date: 2010-09-22 18:39
On Wed, Sep 22, 2010 at 9:39 AM, Jason R. Coombs <report@bugs.python.org> wrote:
> .. It appears __getinitargs__ does not work on Python 2.5 or Python 2.7.

Yes,  __getinitargs__  is only used for old style classes.  I was
wrong on that point.

> Exceptions of the following class still raise a TypeError on unpickling:
>
> class D(Exception):
>    """Extension with values, init called with no args."""
>    def __init__(self, foo):
>        self.foo = foo
>        Exception.__init__(self)
>
>    def __getinitargs__(self):
>        return self.foo,
>

The problem with your D class is that it does not provide correct args
attribute which is expected from an exception class:

()

> Using __reduce__ does seem to work. I suspect this is because Exceptions are extension types.
>

There are two ways to fix this class.  Both fix the args issue as well:

1. Pass foo to Exception.__init__  like this: Exception.__init__(self,
foo) in D.__init__.

2. Explicitly initialize self.args: self.args	= foo,

> I think the fundamental problem is that pickling exceptions does not follow the principle of least surprise. In particular:
>
>  - Other built-in objects (dicts, lists, etc) don't require special handling (replace Exception with dict in the above example and it works).

Other built-in objects don't provide an API for retrieving their init arguments.

>  - It's not obvious how to write an Exception subclass that takes additional arguments and make it pickleable.

AFAICT, this is python subclassing 101: if base class __init__ uses
arguments, they should be passed to it by subclass' __init__.

>  - Neither the pickle documentation nor the Exception documentation describe how pickling is implemented in Exceptions.
>

Exception documentation may be improved by adding a section on
subclassing.  Note that the args argument was not even mentioned in
2.7 documentation, so some discussion of its role may be helpful
somewhere.

> Eric has provided some good use cases. Furthermore, it seems counter-intuitive to me to pass a subclass' custom
> arguments to the parent class. Indeed, even the official tutorial defines exception classes that are unpickleable
> (http://docs.python.org/tutorial/errors.html#tut-userexceptions).
>

Well, the tutorial examples should probably be changed.  In these
examples base class __init__ is not called at all which is probably
not a good style.

> If the use case is obvious enough that it shows up in the hello world tutorial, I don't think
> there should be any argument that it's not a common use case.
>

I am not arguing against simplifying Exception subclassing.  I just
don't see an easy solution that would not promote subclasses with
unusable args attribute.  I also disagree with this issue classified
as a bug.  It may be a valid feature request, but not a bug.

In any case, no proponent of this feature has come up with a patch for
3.2 so far and in my view, this would be a prerequisite for moving
this forward.

> At the very least, there should be a section in the pickle documentation or Exception
> documentation describing how one should make a pickleable subclass. ..

I agree, but again someone has to step in to write such section.
Improving documentation may also be the only solution for the 2.x
series.
msg133581 - (view) Author: Ben Bass (bpb) Date: 2011-04-12 13:52
Perhaps this should be addressed separately, but subprocess.CalledProcessError is subject to this problem (can't be unpickled) (it has separate returncode and cmd attributes, but no args).

It's straightforward to conform user-defined Exceptions to including .args and having reasonable __init__ functions, but not possible in the case of stdlib exceptions.

>>> import subprocess, pickle
>>> try:
...   subprocess.check_call('/bin/false')
... except Exception as e:
...   pickle.loads(pickle.dumps(e))
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/usr/lib/python3.1/subprocess.py", line 435, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '/bin/false' returned non-zero exit status 1

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
  File "/usr/lib/python3.1/pickle.py", line 1363, in loads
    encoding=encoding, errors=errors).load()
TypeError: __init__() takes at least 3 positional arguments (1 given)
msg151039 - (view) Author: Faheem Mitha (fmitha) Date: 2012-01-11 08:01
What is the status on this? It contains to be an
issue. See http://bugs.python.org/issue13751 and
http://bugs.python.org/issue13760
msg151068 - (view) Author: Tal Einat (taleinat) (Python committer) Date: 2012-01-11 16:57
I'd just like to weigh in and say that this is a major issue for me at the moment. Not being able to indiscriminately pickle/unpickle exceptions is making my parallel-processing work very painful, because of problematic stdlib exceptions.

I'm surprised this hasn't been fixed way back in 2.x. FWIW, for my project having this fixed in 3.x could be a significant incentive to finally ditch 2.x.
History
Date User Action Args
2012-03-11 10:17:28eric.araujosetnosy: + pitrou
2012-03-08 22:01:28lukasz.langasetassignee: lukasz.langa

nosy: + lukasz.langa
2012-01-16 15:17:58zbyszsetnosy: + zbysz
2012-01-13 20:53:28georg.brandlsetassignee: georg.brandl -> (no value)
2012-01-11 16:57:09taleinatsetnosy: + taleinat
messages: + msg151068
2012-01-11 08:01:58fmithasetnosy: + fmitha
messages: + msg151039
2011-06-12 18:35:56terry.reedysetversions: + Python 3.3, - Python 3.1
2011-04-12 13:52:51bpbsetnosy: + bpb
messages: + msg133581
2011-04-05 11:34:02hayposetnosy: + haypo
2010-09-22 18:39:40belopolskysetmessages: + msg117149
2010-09-22 14:58:05jason.coombssetmessages: + msg117145
2010-09-22 13:39:06jason.coombssetnosy: + jason.coombs
messages: + msg117139
2010-09-17 12:58:19BreamoreBoysetversions: + Python 2.7, Python 3.2, - Python 2.6
2010-06-30 03:04:12ehusssetmessages: + msg108960
2010-06-29 22:18:58belopolskysetnosy: + belopolsky
messages: + msg108954
2010-05-31 21:13:58tseaversetfiles: + issue1692335-tests.patch
nosy: + tseaver
messages: + msg106811

2010-04-13 00:22:15kylevsetnosy: + kylev
messages: + msg102995
2009-05-15 02:22:51ajaksu2setstage: patch review
type: behavior
versions: + Python 3.1, - Python 2.5
2008-11-18 23:07:21ehusssetmessages: + msg76028
2008-11-18 22:54:39benjamin.petersonsetnosy: - benjamin.peterson
2008-11-18 22:52:19gvanrossumsetstatus: closed -> open
resolution: out of date ->
messages: + msg76026
2008-11-18 22:06:07ehusssetmessages: + msg76020
2008-11-17 17:46:34gvanrossumsetstatus: open -> closed
resolution: out of date
2008-11-17 11:37:01zseilsetmessages: + msg75955
2008-07-31 01:51:23benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg70453
2008-02-04 19:44:49alexandre.vassalottisetnosy: + alexandre.vassalotti
2008-02-03 19:04:04loewissetnosy: + loewis
messages: + msg62025
2008-02-02 11:36:55gvanrossumsetmessages: + msg61999
2008-02-02 08:07:22jarpasetmessages: + msg61997
2008-02-01 17:38:02gvanrossumsetnosy: + gvanrossum
messages: + msg61967
2008-01-19 14:03:52facundobatistasetnosy: + facundobatista
messages: + msg60163
2008-01-17 22:13:52jarpasetfiles: + exception_pickling_26.diff
nosy: + jarpa
messages: + msg60067
2007-11-20 02:00:03brett.cannonsetnosy: + brett.cannon
2007-09-20 08:47:33georg.brandlsetmessages: + msg56054
2007-09-17 10:01:29jafosetassignee: georg.brandl
messages: + msg55958
nosy: + jafo
2007-08-28 08:40:38jafosetmessages: - msg52358
2007-08-28 08:40:18jafosetmessages: - msg52354
2007-08-23 17:29:54georg.brandlsetpriority: normal -> critical
versions: + Python 2.6
messages: + msg55165
severity: normal -> major
title: Move initial args assignment to BaseException.__new__ -> Fix exception pickling: Move initial args assignment to BaseException.__new__
2007-08-23 17:28:00georg.brandllinkissue1744398 superseder
2007-08-23 17:24:47georg.brandllinkissue1742889 superseder
2007-04-01 13:46:15zseilcreate