Make HttpStatusError and RequestError pickleable

[hoodmane]

If an error has keyword-only arguments, it needs this extra __reduce__ method to ensure that unpickling doesn't raise
TypeError: missing (n) required keyword-only argument(s).

Checklist

• I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
• I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
• I've updated the documentation accordingly.

[tomchristie]

Are we able to be consistent with how pickle is supported on Request/Response here?

[hoodmane]

All of the key word only arguments of Request have default values so that's why it's pickleable. Our choices here are:

1. Give all kwonly arguments defaults. Classes where all kwonly arguments have defaults mostly have the correct pickle behavior without any special support.
2. Do something like what I have here.

As an aside, this is a bit of a Python wart, ideally simple classes with required kwonly constructor args should be pickleable out of the box. I wonder if there is any discussion on discuss.python.org about it...

[tomchristie]

Okay sure thing.
Thanks for the fix. ☺️

[tomchristie]

Worth adding a CHANGELOG entry for this.

[hoodmane]

Okay updated changelog.

[camillol]

The root issue is that BaseException has a custom reduce that can't handle subclasses with required keyword-only arguments. Overriding reduce is the right fix (barring a fix in Python), but I think it only needs to be in HttpStatusError. RequestError can already be pickled because its keyword argument has a default.

More importantly, I'd like to see this merged!

[tomchristie]

More importantly, I'd like to see this merged!

Okay, I'll rephrase my reservations and let's see if we can get this unblocked.

I'd prefer to accept a __getstate__/__setstate__ implementation to handle the pickling.

See https://docs.python.org/3/library/pickle.html#object.__setstate__

Although powerful, implementing reduce() directly in your classes is error prone. For this reason, class designers should use the high-level interface (i.e., getnewargs_ex(), getstate() and setstate()) whenever possible.

[hoodmane]

I see, I didn't understand from what you said previously that this PR was waiting on me. I'll look into it.

[hoodmane]

No it is not possible to implement this with __getstate__ and __setstate__. object.__reduce__ is responsible for calling __getstate__, but BaseException.__reduce__ does not call __getstate__ -- all it does is directly return a tuple:

static PyObject *
BaseException_reduce(PyBaseExceptionObject *self, PyObject *Py_UNUSED(ignored))
{
    if (self->args && self->dict)
        return PyTuple_Pack(3, Py_TYPE(self), self->args, self->dict);
    else
        return PyTuple_Pack(2, Py_TYPE(self), self->args);
}

So in order to fix the behavior we'll have to override BaseException.__reduce__.

The ordinary object.__reduce__ is implemented here, it just calls _common_reduce directly above which just calls reduce_newobj which finally implements the actual logic. On line 6266 reduce_newobj calls a function named object_getstate. object_getstate looks up __getstate__ and calls it if present, if absent it falls back to object_getstate_default.

Anyways none of this happens if __reduce__ is overridden and doesn't call super().__reduce__, such as when inheriting from BaseException.

[hoodmane]

As @camillol said:

The root issue is that BaseException has a custom __reduce__ that can't handle subclasses with required keyword-only arguments. Overriding reduce is the right fix (barring a fix in Python)

[hoodmane]

@tomchristie can you read my explanation above about why need to override __reduce__ and see if it makes sense? Would be nice to merge this.