[Patches] [ python-Patches-1692335 ] Move initial args assignment to BaseException.__new__

[SourceForge.net]

Patches item #1692335, was opened at 2007-04-01 13:46
Message generated for change (Comment added) made by gbrandl
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1692335&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Core (C code)
Group: Python 2.5
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Ziga Seilnacht (zseil)
Assigned to: Nobody/Anonymous (nobody)
Summary: Move initial args assignment to BaseException.__new__

Initial Comment:
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.



----------------------------------------------------------------------

>Comment By: Georg Brandl (gbrandl)
Date: 2007-08-12 13:24

Message:
Logged In: YES 
user_id=849994
Originator: NO

Yes, this is what I meant with my last comment.

----------------------------------------------------------------------

Comment By: Ziga Seilnacht (zseil)
Date: 2007-08-12 12:34

Message:
Logged In: YES 
user_id=1326842
Originator: YES

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


----------------------------------------------------------------------

Comment By: Georg Brandl (gbrandl)
Date: 2007-08-12 12:13

Message:
Logged In: YES 
user_id=849994
Originator: NO

I wouldn't care too much about .message; it was new in 2.5 and is already
deprecated in 2.6.

----------------------------------------------------------------------

Comment By: Ziga Seilnacht (zseil)
Date: 2007-08-12 11:24

Message:
Logged In: YES 
user_id=1326842
Originator: YES

File Added: exception_pickling_26.diff

----------------------------------------------------------------------

Comment By: Ziga Seilnacht (zseil)
Date: 2007-08-12 11:23

Message:
Logged In: YES 
user_id=1326842
Originator: YES

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

----------------------------------------------------------------------

Comment By: Georg Brandl (gbrandl)
Date: 2007-08-12 08:01

Message:
Logged In: YES 
user_id=849994
Originator: NO

The only question left is what to do if something reassigns
exceptioninstance.args.

----------------------------------------------------------------------

Comment By: Georg Brandl (gbrandl)
Date: 2007-08-12 08:00

Message:
Logged In: YES 
user_id=849994
Originator: NO

The only question left is what to do if something reassigns
exceptioninstance.args.

----------------------------------------------------------------------

Comment By: Eric Huss (ehuss)
Date: 2007-08-12 01:33

Message:
Logged In: YES 
user_id=393416
Originator: NO

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!


----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2007-08-11 18:27

Message:
Logged In: YES 
user_id=33168
Originator: NO

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.

----------------------------------------------------------------------

Comment By: Georg Brandl (gbrandl)
Date: 2007-08-11 09:06

Message:
Logged In: YES 
user_id=849994
Originator: NO

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

----------------------------------------------------------------------

Comment By: Georg Brandl (gbrandl)
Date: 2007-08-11 09:06

Message:
Logged In: YES 
user_id=849994
Originator: NO

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

----------------------------------------------------------------------

Comment By: Eric Huss (ehuss)
Date: 2007-06-27 18:45

Message:
Logged In: YES 
user_id=393416
Originator: NO

Added patch #1744398 as an alternate solution.


----------------------------------------------------------------------

Comment By: Eric Huss (ehuss)
Date: 2007-06-15 00:34

Message:
Logged In: YES 
user_id=393416
Originator: NO

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)


----------------------------------------------------------------------

Comment By: Ziga Seilnacht (zseil)
Date: 2007-04-01 21:50

Message:
Logged In: YES 
user_id=1326842
Originator: YES

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

----------------------------------------------------------------------

Comment By: Ziga Seilnacht (zseil)
Date: 2007-04-01 13:47

Message:
Logged In: YES 
user_id=1326842
Originator: YES

File Added: exc_args_25.diff

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1692335&group_id=5470