Allow pickling HTTPStatusError

[camillol]

Summary

HTTPStatusError's __init__ method has required keyword-only arguments. BaseException has a custom __reduce__ method that can't handle that correctly. As a result, attempting to unpickle a pickled HTTPStatusError fails with:

TypeError: HTTPStatusError.init() missing 2 required keyword-only arguments: 'request' and 'response'

This PR fixes that by defining an appropriate __reduce__ method for HTTPStatusError.

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.

Note: I don't think this requires a documentation change, but let me know if so.

[camillol]

There is an earlier PR at #3108. We just need a maintainer to merge either of them.

[kaxil]

Hello, Apache Airflow maintainer here. We are facing the same issue: apache/airflow#47873 . We have adopted httpx fully and love it and we are in the middle of releasing Airflow 3.0. This bug is causing us to not propogoate the error message from our API server back to the task process.

Would love for one of these or #3108 merged.

cc @tomchristie @zanieb @karpetrosyan

[zanieb]

@camillol can you explain why this approach is preferable to #3108?

[camillol]

@zanieb I don't know if it's inherently preferable, but I can explain why I wrote it that way.

For a generic object, the default unpickle behavior is to call __new__ and restore the __dict__. That would be similar to having this:

def __reduce__(self):
    return (self.__class__.__new__, (self.__class__,), self.__dict__)

However, BaseException defines a custom __reduce__. In Python, it would be equivalent to this:

def __reduce__(self):
    return (self.__class__, (self.args,), self.__dict__)

So, for exceptions, __init__ is called during unpickling. But because of the way __reduce__ is defined, it cannot handle required keyword arguments.

My goal in this PR was to make unpickling work while keeping the behavior expected from exceptions. Therefore, I still call self.__class__, which calls __init__, and I make sure to pass it the keyword arguments. I still include __dict__ in the state, in case there are custom attributes on the object, but I exclude request and response from it since they are already passed to __init__.

#3108 takes a different approach. It calls __new__ directly to unpickle, which is closer to the default unpickle behavior for generic objects, but is different from the normal behavior for exceptions, because it does not call __init__. (Also, it uses Exception.__new__ instead of self.__class__.__new__, which seems like an unnecessary behavior from even the plain-object behavior).

To sum up: this PR is more verbose, but less risky, because it hews closer to normal exception unpickling. If there is something that relies on exceptions calling __init__ during unpickling, now or in the future, it will work correctly with this PR. If someone overrides __new__, it will also work correctly, etc.

However, you might instead make the judgement call that these subtle behavior changes are unlikely to be a problem with the exceptions that you have now, and go for the shorter solution in #3108. That's fine too, although I would still at least replace Exception.__new__ with self.__class__.__new__.

[zanieb]

Great, thank you for taking the time to write that up in detail. This seems preferable to me.

[zanieb]

(I can't unilaterally release this)

[camillol]

I don't have merge permissions, either. Is there something else I need to do? Who has the merge button?

[zanieb]

I could merge it, but @tomchristie will need to weigh-in before it can be released regardless.

[lovelydinosaur]

This seems bizarrely inelegant to me. Overriding a magic method that I've never heard of prior to this issue, that's documented in stdlib with a warning. Seems like a gnarly code smell.

It's not obvious that we should necessarily make exceptions pickleable. User code could choose to deal with that at a wrapping level.

[zanieb]

Overriding a magic method that I've never heard of prior to this issue, that's documented in stdlib with a warning. Seems like a gnarly code smell.

It's not obvious that we should necessarily make exceptions pickleable. User code could choose to deal with that at a wrapping level.

I disagree. I've had to do this before, use of __reduce__ is not that exceptional. The majority of exceptions are pickleable and it's surprising that this one httpx type is not. It's non-trivial for users to deal with wrapping this exception into a pickleable type.

If you're particularly opposed to this approach... maybe we should remove the keyword-only arguments from the exception type instead? Then it'd be pickleable without implementing __reduce__, I presume? I'm not sure enforcing keyword-only arguments there is worth the complexity they're adding.

[kaxil]

I disagree. I've had to do this before, use of __reduce__ is not that exceptional. The majority of exceptions are pickleable and it's surprising that this one httpx type is not. It's non-trivial for users to deal with wrapping this exception into a pickleable type.

If you're particularly opposed to this approach... maybe we should remove the keyword-only arguments from the exception type instead? Then it'd be pickleable without implementing __reduce__, I presume? I'm not sure enforcing keyword-only arguments there is worth the complexity they're adding.

💯 % agreed !

[yueyinqiu]

Hello. Anything new about this? This issue prevents correct exception information from being retrieved in multi-process scenarios, resulting in something like TypeError: APIStatusError.__init__() missing 2 required keyword-only arguments: 'response' and 'body'. However, multi-process is frequently used for network requests, and I believe this is an issue this project need to consider.

Creating a custom type is possible, but without guidance on how to wrap it, some exception information may be lost, which may not be desirable. Also, while this may be a design issue within Python itself, Python's multi-process API is intended to provide a completely transparent interface. In theory, it is expected to be possible to use multi-process without modifying any other code.

[oelhammouchi]

Any news on this?

[camillol]

@lovelydinosaur in #3108 (comment) you asked if this could be implemented with __getstate__ instead. Not sure if you saw the replies there; unfortunately the discussion is split across two PRs. To sum up:

• This cannot be handled with __getstate__/__setstate__.
• Overriding __reduce__ is documented and supported. The docs do say that "class designers should use the high-level interface [...] whenever possible. We will show, however, cases where using __reduce__() is the only option". But in this case, the designers of BaseException chose to implement the __reduce__ interface, and that forces us to use it too; this is one of the cases where "__reduce__() is the only option".
• The alternative would be to change these exception classes not to have required keyword arguments (because BaseException's __reduce__ cannot handle them); it seems less ideal to change their API, but it would also solve the problem.

I think everyone would be happy with any of the following:

• merging this PR;
• merging Make HttpStatusError and RequestError pickleable #3108 instead;
• merging another implementation of __reduce__ of your preference;
• merging a PR that makes the keyword arguments optional.

We're happy with any solution you prefer, as long as the tests in this PR pass.