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.

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

Jump to bottom

internal/retry: wrapped errors have grpc status code OK when the wrapped error is not a gRPC error #8127

Closed
atollena opened this issue Jun 16, 2023 · 2 comments
Closed

internal/retry: wrapped errors have grpc status code OK when the wrapped error is not a gRPC error #8127

atollena opened this issue Jun 16, 2023 · 2 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@atollena
Copy link
Contributor

atollena commented Jun 16, 2023 •
edited
Loading

Client

All clients that use retries.

Environment

Any.

Go Environment

Any.

Code

Any code using internal.Retry().

Expected behavior

An error's GRPCStatus() method should never returns nil, which maps to a status with Code.OK (https://github.com/grpc/grpc-go/blob/642dd63a85275a96d172f446911fd04111e2c74c/internal/status/status.go#L71-L76). Instead it should return an actual error code, such as Status.Unknown.

Actual behavior

If Status.FromError returns !ok, which it does if the wrapped error is not a gRPC error, then the wrapped error has GRPCStatus() returning nil (Status.OK).

Additional context

We discovered this problem because it can cause panics in recent versions of grpc-go. See this issue: grpc/grpc-go#6373.
@atollena atollena added the triage me I really want to be triaged. label Jun 16, 2023
atollena added a commit to atollena/google-cloud-go that referenced this issue Jun 16, 2023
@atollena
internal/retry: Never return nil from GRPCStatus()
3906f17 
An error's GRPCStatus() method should never returns nil, which maps to a status with
Code.OK (https://github.com/grpc/grpc-go/blob/642dd63a85275a96d172f446911fd04111e2c74c/internal/status/status.go#L71-L76). Instead
it should return an actual error code, such as Status.Unknown.

This addresses googleapis#8127.
atollena added a commit to atollena/google-cloud-go that referenced this issue Jun 16, 2023
@atollena
internal/retry: Never return nil from GRPCStatus()
04b8155 
An error's GRPCStatus() method should never returns nil, which maps to a status with
Code.OK (https://github.com/grpc/grpc-go/blob/642dd63a85275a96d172f446911fd04111e2c74c/internal/status/status.go#L71-L76). Instead
it should return an actual error code, such as Status.Unknown.

This addresses googleapis#8127.
@atollena atollena mentioned this issue Jun 16, 2023
Merged
@atollena atollena changed the title internal/retry: wrapped errors may have grpc.Status OK internal/retry: wrapped errors have grpc status code OK when the wrapped error is not a gRPC error Jun 16, 2023
atollena added a commit to atollena/google-cloud-go that referenced this issue Jun 16, 2023
@atollena
fix[internal/retry]: Never return nil from GRPCStatus()
57b4f8e 
An error's GRPCStatus() method should never returns nil, which maps to a status with
Code.OK (https://github.com/grpc/grpc-go/blob/642dd63a85275a96d172f446911fd04111e2c74c/internal/status/status.go#L71-L76). Instead
it should return an actual error code, such as Status.Unknown.

This addresses googleapis#8127.
atollena added a commit to atollena/google-cloud-go that referenced this issue Jun 16, 2023
@atollena
fix: Never return nil from GRPCStatus() in internal/retry
dff8bf9 
An error's GRPCStatus() method should never returns nil, which maps to a status with
Code.OK (https://github.com/grpc/grpc-go/blob/642dd63a85275a96d172f446911fd04111e2c74c/internal/status/status.go#L71-L76). Instead
it should return an actual error code, such as Status.Unknown.

This addresses googleapis#8127.
@noahdietz noahdietz added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p3 Desirable enhancement or fix. May not be included in next release. and removed triage me I really want to be triaged. labels Jun 16, 2023
@noahdietz
Copy link
Contributor

Thanks for reporting this. Let's take a look at your fix!

@noahdietz noahdietz added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed priority: p3 Desirable enhancement or fix. May not be included in next release. labels Jun 16, 2023
@atollena
Copy link
Contributor Author

Fixed by 005d2df.

@atollena atollena mentioned this issue Jun 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@atollena @noahdietz