Skip to content

bpo-27015: Save kwargs given to exceptions constructor #11580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

bpo-27015: Save kwargs given to exceptions constructor #11580

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6a00f21
Save kwargs given to exceptions constructor
Jan 15, 2019
12dc912
fix NEWS entry typo and formatting
gpshead Jan 16, 2019
1d12d28
Move CalledProcessError pickling test to test_subprocess
Jan 16, 2019
88ba13e
Fix tuple size
Jan 16, 2019
9986e86
Create self.kwargs only if accessed
Jan 16, 2019
7794862
Fix coding style issues and use Python memory allocator
Jan 28, 2019
6c906da
Implement BaseException.__getnewargs_ex__
Feb 20, 2019
4657d7e
Use __reduce_ex__ and __getnewargs_ex__
Feb 20, 2019
2fc3366
Remove some Refleaks
Feb 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix coding style issues and use Python memory allocator
  • Loading branch information
Rémi Lapeyre committed Jan 28, 2019
commit 7794862518703546a1f73cc9df889ba08845b91e
25 changes: 17 additions & 8 deletions Objects/exceptions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,11 @@ BaseException_repr(PyBaseExceptionObject *self)
}
key = PyTuple_GET_ITEM(item, 0);
value = PyTuple_GET_ITEM(item, 1);
PyTuple_SET_ITEM(seq, i, PyUnicode_FromFormat("%S=%R", key, value));
repr = PyUnicode_FromFormat("%S=%R", key, value);
if (repr == NULL) {
goto fail;
}
PyTuple_SET_ITEM(seq, i, repr);
i++;
Py_DECREF(item);
}
Expand Down Expand Up @@ -221,36 +225,41 @@ BaseException_reduce(PyBaseExceptionObject *self, PyObject *Py_UNUSED(ignored))
PyObject *functools;
PyObject *partial;
PyObject *constructor;
PyObject *args;
PyObject *result;
PyObject **newargs;

_Py_IDENTIFIER(partial);
functools = PyImport_ImportModule("functools");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reduce implementation concerns me, as it looks like it will make everything much slower, even for exception instances where self->kwargs isn't set.

Instead, I'd recommend migrating BaseException away from implementing __reduce__ directly, and instead have it implement __getnewargs_ex__: https://docs.python.org/3/library/pickle.html#object.__getnewargs_ex__

That way the pickle machinery will take care of calling __new__ with the correct arguments, and you wouldn't need to introduce a weird dependency from a core builtin into a standard library module.

(That would have potential backwards compatibility implications for subclasses implementing reduce based on the parent class implementation, but the same would hold true for introduce a partial object in place of a direct reference to the class - either way, there'll need to be a note in the Porting section of the What's New guide, and switching to __get_newargs_ex__ will at least have the virtue of simplifying the code rather than making it more complicated)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do that, I removed __reduce__ and added __getnewargs_ex__ to the methods as:

static PyObject *
BaseException_getnewargs_ex(PyBaseExceptionObject *self, PyObject *Py_UNUSED(ignored))
{
    PyObject *args = PyObject_GetAttrString((PyObject *) self, "args");
    PyObject *kwargs = PyObject_GetAttrString((PyObject *) self, "kwargs");

    if (args == NULL || kwargs == NULL) {
        return NULL;
    }

    return Py_BuildValue("(OO)", args, kwargs);
}

but it brocke pickling. Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, found my mistake, using __getnewargs_ex__ broke pickling for protocols 0 and 1. Is this expected?

I don't think this happened when using a partial reference on the the constructor of the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's ok to broke pickling support for protocols 0 and 1 since it was broken for keyword args anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining __reduce_ex__ would let you restore the old behaviour for those protocols, but I'm not sure __getnewargs_ex__ will still be called if you do that (we're reaching the limits of my own pickle knowledge).

@pitrou Do you have any recommendations here? (Context: trying to get BaseException to pickle keyword args properly, wanting to use __getnewargs_ex__ for more recent pickle protocols, but wondering how to handle the older protocols that don't use that)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should I call object.__reduce_ex__?

It seems to me that calling the builtin super is not done anywhere in the source code but I don't find the right way to do it.

Do I need to call object___reduce_ex__ directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ncoghlan Well, I'm not sure why you wouldn't implement the entire logic in __reduce_ex__, instead of also defining __getnewargs_ex__?

Or, rather, you could just define __getnewargs_ex__ and stop caring about protocols 0 and 1 (which are extremely obsolete by now, so we want to maintain compatibility, but fixing bugs in them is not important).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pitrou I only suggested delegating to __getnewargs_ex__ because I wasn't sure how to mimic that behaviour from inside a custom __reduce_ex__ implementation.

But if __reduce__ still gets called for protocols 0 and 1 even when __getnewargs_ex__ is defined, then that's even better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pitrou @ncoghlan, thanks for you input. I pushed a new commit that implement __getnewargs_ex__ but it seems that __reduce_ex__ does not check it and call __reduce__ no matter what the protocol is:

>>> BaseException().__reduce_ex__(0)
(<class 'BaseException'>, ())
>>> BaseException().__reduce_ex__(1)
(<class 'BaseException'>, ())
>>> BaseException().__reduce_ex__(2)
(<class 'BaseException'>, ())
>>> BaseException().__reduce_ex__(3)
(<class 'BaseException'>, ())
>>> BaseException().__reduce_ex__(4)
(<class 'BaseException'>, ())
>>> BaseException().__getnewargs_ex__()
((), {})

If I remove the __reduce__, then it breaks pickling for protocols 0 and 1:

>>> BaseException().__reduce_ex__(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/remi/src/cpython/Lib/copyreg.py", line 66, in _reduce_ex
    raise TypeError(f"cannot pickle {cls.__name__!r} object")
TypeError: cannot pickle 'BaseException' object
>>> BaseException().__reduce_ex__(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/remi/src/cpython/Lib/copyreg.py", line 66, in _reduce_ex
    raise TypeError(f"cannot pickle {cls.__name__!r} object")
TypeError: cannot pickle 'BaseException' object
>>> BaseException().__reduce_ex__(2)
(<function __newobj__ at 0x105c63040>, (<class 'BaseException'>,), None, None, None)
>>> BaseException().__reduce_ex__(3)
(<function __newobj__ at 0x105c63040>, (<class 'BaseException'>,), None, None, None)
>>> BaseException().__reduce_ex__(4)
(<function __newobj__ at 0x105c63040>, (<class 'BaseException'>,), None, None, None)

Do I need to define a custom __reduce_ex__ as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug further and it seems my issue comes from https://github.com/python/cpython/blob/master/Lib/copyreg.py#L66, I will look into the details tomorrow.

if (!functools)
if (functools == NULL) {
return NULL;
}
partial = _PyObject_GetAttrId(functools, &PyId_partial);
Py_DECREF(functools);
if (!partial)
if (partial == NULL) {
return NULL;
}

Py_ssize_t len = 1;
if (PyTuple_Check(self->args)) {
len += PyTuple_GET_SIZE(self->args);
}
newargs = PyMem_RawMalloc(len*sizeof(PyObject*));
newargs = PyMem_New(PyObject *, len);
if (newargs == NULL) {
PyErr_NoMemory();
return NULL;
}
newargs[0] = (PyObject *)Py_TYPE(self);

for (Py_ssize_t i=1; i < len; i++) {
newargs[i] = PyTuple_GetItem(self->args, i-1);
}
constructor = _PyObject_FastCallDict(partial, newargs, len, self->kwargs);
PyMem_RawFree(newargs);
PyMem_Free(newargs);

Py_DECREF(partial);

args = PyTuple_New(0);
if (!args) {
PyObject *args = PyTuple_New(0);
if (args == NULL) {
return NULL;
}
if (self->args && self->dict){
Expand Down