yuqi1129 commented on code in PR #14366:
URL: https://github.com/apache/gravitino/pull/14366#discussion_r4424536695
##########
clients/client-python/gravitino/exceptions/base.py:
##########
@@ -30,6 +30,27 @@ def __init__(self, message, *args):
super().__init__(message % args)
+class HTTPResponseException(RESTException):
+ """An HTTP failure whose response cannot be used as a domain error.
+
+ Attributes:
+ status_code: The HTTP status received from the server.
+ request_method: The original client request's HTTP method.
+ endpoint: The request origin, without credentials, path or query
parameters.
+ """
+
+ def __init__(
+ self, status_code: int, request_method: str, endpoint: str, reason: str
+ ):
+ """Create an error from safe context and a fixed diagnostic reason."""
+ self.status_code = status_code
+ self.request_method = request_method
+ self.endpoint = endpoint
+ super().__init__(
Review Comment:
[Important] Make the new exception reconstructible by pickle/multiprocessing.
This constructor requires four arguments, but `RESTException.__init__`
leaves only the formatted message in `BaseException.args`. Pickle reconstructs
the exception with that one argument before restoring its attributes, so this
fails:
```python
pickle.loads(pickle.dumps(
HTTPResponseException(502, "GET", "http://localhost";, "Empty error
response")
))
# TypeError: ... missing 3 required positional arguments ...
```
I also reproduced the consequence with a real Python 3.11.14
`ProcessPoolExecutor` worker invoking `HTTPClient.get` against a controlled
empty HTTPError: `Future.result()` raises `BrokenProcessPool` instead of the
HTTP exception, breaking the application's process pool. Reverting only
HTTPClient to the base makes the same worker return the ordinary
`RESTException` successfully.
Please retain the safe reason/context and provide reconstruction, for
example an explicit `__reduce__` supplying the constructor arguments. Add a
pickle round-trip and a cross-process exception propagation regression,
preserving type/message/status/method/origin.
Validation used the public HTTPClient path with a synthetic empty 502 and an
actual standard-library process pool. I did not identify an existing repository
multiprocessing HTTPClient caller or test Ray/cloudpickle; this is a reproduced
external SDK consumer, not a claim that the current Ray tests hit it.
##########
clients/client-python/gravitino/utils/http_client.py:
##########
@@ -134,25 +139,84 @@ def _make_request(self, opener, request, timeout=None) ->
Tuple[bool, Response]:
try:
return (True, Response(opener.open(request, timeout=timeout)))
except HTTPError as err:
+ try:
+ return (False, self._read_error_response(err, request))
+ finally:
+ try:
+ err.close()
+ except OSError:
+ # Closing a failed response must not replace its HTTP
failure.
+ pass
+
+ def _read_error_response(self, err, request) -> ErrorResponse:
+ try:
err_body = err.read()
+ except (IncompleteRead, OSError):
+ raise self._http_response_exception(
+ err, request, "Unable to read error response"
+ ) from None
- if err_body is None or len(err_body) == 0:
- return (
- False,
- ErrorResponse.generate_error_response(RESTException,
err.reason),
- )
+ if not err_body:
+ raise self._http_response_exception(
+ err, request, "Empty error response"
+ ) from None
- err_resp = self._parse_error_response(err_body)
- err_resp.validate()
+ try:
+ response = self._parse_error_response(err_body)
+ response.validate()
+ except (ValueError, TypeError, KeyError, BadRequestException):
Review Comment:
[Important] Handle the JSON decoder's `RecursionError` through the safe
fallback as well.
`json.loads` can raise `RecursionError` for excessive nesting, which is
outside this catch. A roughly 3 KB error body is enough with the ordinary
recursion limit:
```python
body = b"[" * 1500 + b"]" * 1500
```
Through the current public `HTTPClient.get` path, an HTTP 502 with that body
raises `RecursionError`, without `status_code`, instead of
`HTTPResponseException`. Its default traceback also retains the HTTPError
context: a synthetic HTTP reason `private-reason` appears in the formatted
traceback, bypassing the fixed-reason/suppressed-chain protection. The response
does still close in `finally`.
Please catch `RecursionError` specifically within this parse/validation
boundary (or reject excessive nesting safely), then raise the same fixed-reason
fallback `from None`. Keep custom handler failures outside the catch. Add a
nested-body regression checking the SDK exception, HTTP context, closed
response and absence of the private HTTP reason/underlying chain.
Verified on Python 3.11.14 with the actual decoder and default traceback
formatting, using a controlled HTTPError rather than a live gateway. The base
also exposes RecursionError, so this is an uncovered case of the intended
malformed-response fix, not a newly introduced parsing regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
