Patches item #1692335, was opened at 2007-04-01 15:46 Message generated for change (Comment added) made by zseil 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: Ziga Seilnacht (zseil) Date: 2007-08-12 14: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 14: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 13: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 13: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 10: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 10: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 03: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 20: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 11: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 11: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 20: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 02: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 23: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 15: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 _______________________________________________ Patches mailing list Patches@python.org http://mail.python.org/mailman/listinfo/patches
