Skip to content
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.

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

L105: Python Add New Error Types #388

Merged
merged 4 commits into from Sep 27, 2023

Conversation

XuanWang-Amos
Copy link
Contributor

@XuanWang-Amos XuanWang-Amos commented Aug 21, 2023

Add two new errors in grpc public API.
Discussion thread: https://groups.google.com/g/grpc-io/c/pG34X9nAa3c

Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

LGTM. But please do not merge until 2 weeks after the thread has been posted to the mailing list.

## Rationale

The Async API [has similar errors](https://github.com/grpc/grpc/blob/v1.57.x/src/python/grpcio/grpc/aio/__init__.py#L23,L24). We're refactoring code so those errors will also be used in Sync API. Adding them to the Sync API will help us keep the two stacks in sync and allow users of the Sync implementation to catch and handle aborts.

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to mention that adding this will allow users to type:

try:
  do_grpc_stuff()
except grpc.BaseError as e:
  # handle error

and be confident that they're catching all gRPC exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also have RpcError exposed to user, I think we should mention their differences, when should user expect RpcError?

Copy link
Contributor

Choose a reason for hiding this comment

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

If RpcError is not currently a subclass of BaseError, I think we should make it a subclass of BaseError. Ditto for all other exception types. The ultimate goal is to enable the snippet above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense, I'll change RpcError to be a subclass of BaseError and add it to this gRFC.

L105-python-expose-new-error-types.md Outdated Show resolved Hide resolved
@XuanWang-Amos XuanWang-Amos merged commit bf2023d into grpc:master Sep 27, 2023
1 check passed
@XuanWang-Amos XuanWang-Amos deleted the expose_new_error_type branch September 27, 2023 16:46
XuanWang-Amos added a commit to grpc/grpc that referenced this pull request Sep 27, 2023
Fix: #33890

In case of abort, currently we don't log anything, this change added an
`AbortError` as the default error for abort, if any other error
happened, we'll log the error so user will be aware of the other error.

Related gRFC: grpc/proposal#388

### Testing
* Tested locally, after this change, any non-AbortError will be logged,
sample log:
```
ERROR:grpc._server:Exception happened while abort: Other exception happened!
Traceback (most recent call last):
  File "/usr/local/google/home/xuanwn/.cache/bazel/_bazel_xuanwn/da3828576aa39e99a5c826cc2e2e22fb/sandbox/linux-sandbox/9/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/bin/src/python/grpcio_tests/tests/unit/_abort_test.native.runfiles/com_github_grpc_grpc/src/python/grpcio/grpc/_server.py", line 553, in _call_behavior
    response_or_iterator = behavior(argument, context)
  File "/usr/local/google/home/xuanwn/.cache/bazel/_bazel_xuanwn/da3828576aa39e99a5c826cc2e2e22fb/sandbox/linux-sandbox/9/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/bin/src/python/grpcio_tests/tests/unit/_abort_test.native.runfiles/com_github_grpc_grpc/src/python/grpcio_tests/tests/unit/_abort_test.py", line 95, in abort_with_status_unary_unary_raise_additional_exception
    raise Exception("Other exception happened!")
Exception: Other exception happened!
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants