classification
Title: unpickling an xmlrpc.client.Fault raises TypeError
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Uri Okrent, berker.peksag, loewis, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-06-09 15:09 by Uri Okrent, last changed 2016-06-09 20:30 by Uri Okrent.

Files
File name Uploaded Description Edit
0001-xmlrpc.client-make-Fault-pickleable.patch Uri Okrent, 2016-06-09 15:09 pass Fault's __init__() args to the base class __init__() review
0001-xmlrpc.client-make-Errors-pickleable.patch Uri Okrent, 2016-06-09 20:30 review
Messages (7)
msg268028 - (view) Author: Uri Okrent (Uri Okrent) * Date: 2016-06-09 15:09
Attempting to unpickle an xmlrpc.client.Fault will raise a TypeError:

>>> import xmlrpc.client as xmlrpclib
>>> f = xmlrpclib.Fault(42, 'Test Fault')
>>> import pickle
>>> s = pickle.dumps(f)
>>> pickle.loads(s)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__() missing 2 required positional arguments: 'faultCode' and 'faultString'

Apparently unpickle relies on BaseException's args attribute when reconstructing an Exception class that inherits from BaseException (Fault inherits from Exception).  Unfortunately, Fault implements __init__() and does call the base class constructor, but does not pass its arguments, so Fault.args is always an empty tuple.

Simply passing Fault's arguments to the base class constructor allows it to be unpickled.

I've included a patch for 3.x but the issue is present in 2.x as well (the code and fix are exactly the same except in xmlrpclib.py).
msg268039 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-06-09 17:08
Looks good to me, thanks.
msg268048 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-09 18:13
Why you need to pickle Fault?
msg268050 - (view) Author: Uri Okrent (Uri Okrent) * Date: 2016-06-09 18:40
I'm not pickling/unpickling it directly, I'm using multiprocessing to handle queries to my server in worker processes which is using pickle to propagate exceptions raised in the worker to the parent.

I could instead raise a different exception and wrap it in a Fault later (which is what I ended up doing to avoid the issue), but it seems like a case of "to-may-to" vs. "to-mah-to".  Either way an Exception should be able to be propagated up to the parent in multiprocessing, whether it's my own or Fault.
msg268052 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-09 19:08
This looks reasonable.

But since Fault never was unpickleable, this issue is in gray zone between bug fixing and adding new feature. The latter can be done only in new Python version. I hesitate with classification of this issue.

There are a lot of other exceptions that can't be unpickled (including xmlrpc.client.ProtocolError). This may be a part of larger issue.

As for the patch itself, it solves the issue. But faultCode and faultString are saved twice (this is not a large problem for multiprocessing). Test should test pickling with all possible protocols (see how other pickle tests are written).
msg268056 - (view) Author: Uri Okrent (Uri Okrent) * Date: 2016-06-09 20:01
My reading of the docs leads me to lean towards bug since this seems to break the contract of BaseException API in a standard lib module:

https://docs.python.org/3/library/exceptions.html#BaseException says BaseExceptions have args and with_traceback so those probably should be preserved in all standard lib inheritors.

This excerpt from the tutorial also seems to imply the same:
https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions

"In this example, the default __init__() of Exception has been overridden. The new behavior simply creates the value attribute. This replaces the default behavior of creating the args attribute."

The tutorial goes out of its way to note that the default behavior of creating args has been replaced in this simple example -- i.e., one would expect args to be present.


As for your other comments, should I make a similar change to other inheritors of xmlrpc.client that inherit from xmlrpc.client.Error?  Should I instead completely remove Fault's constructor and just make faultCode and faultString properties so the attributes aren't duplicated (that seems like it may be dangerous)?

I will update the test for pickling with the other protocols.
msg268058 - (view) Author: Uri Okrent (Uri Okrent) * Date: 2016-06-09 20:30
I updated the patch to improve the tests and made the same change to xmlrpc.client.ProtocolError (which was the only other place in xmlrpc.client).  I'll let you decide if this patch is better despite the fact that it suffers from the same data duplication as Fault.

If not, we can go back to just fixing Fault, or come up with a fancier solution entirely that doesn't duplicate data.  Though, I hesitate to start getting too fancy for two reasons: 1) this module is old and stable, and 2) these are exception objects, so while the data duplication is ugly, the instances themselves should be short-lived.
History
Date User Action Args
2016-06-09 20:30:57Uri Okrentsetfiles: + 0001-xmlrpc.client-make-Errors-pickleable.patch

messages: + msg268058
2016-06-09 20:01:05Uri Okrentsetmessages: + msg268056
2016-06-09 19:08:42serhiy.storchakasetmessages: + msg268052
2016-06-09 18:40:11Uri Okrentsetmessages: + msg268050
2016-06-09 18:13:44serhiy.storchakasetmessages: + msg268048
2016-06-09 17:55:52serhiy.storchakasetnosy: + serhiy.storchaka
2016-06-09 17:08:44berker.peksagsetnosy: + berker.peksag
messages: + msg268039
2016-06-09 16:42:34SilentGhostsetnosy: + loewis
stage: patch review

versions: - Python 2.7, Python 3.2, Python 3.3, Python 3.4
2016-06-09 15:09:16Uri Okrentcreate