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

fix(internal/retry): never return nil from GRPCStatus() #8128

Merged
merged 4 commits into from
Jun 17, 2023
Merged

fix(internal/retry): never return nil from GRPCStatus() #8128

merged 4 commits into from
Jun 17, 2023

Conversation

atollena
Copy link
Contributor

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 #8127.

@atollena atollena requested a review from a team as a code owner June 16, 2023 09:53
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jun 16, 2023
@conventional-commit-lint-gcf
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@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 reviewed Jun 16, 2023
View reviewed changes
Copy link
Contributor

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

This is basically the same change we made to gax-go for a similar reason. Thanks for doing this. One nit.

internal/retry.go Outdated Show resolved Hide resolved
@noahdietz noahdietz requested a review from tritone June 16, 2023 16:56
@noahdietz
Copy link
Contributor

@tritone @codyoss I'm not as familiar with where this package is used. Just want to double check - any issues with this change?

@noahdietz
Copy link
Contributor

@atollena sorry out of habit I caught up your branch 😬 might want to pull before pushing a new commit

@noahdietz noahdietz changed the title internal/retry: Never return nil from GRPCStatus() fix(internal/retry): never return nil from GRPCStatus() Jun 16, 2023
@codyoss
Copy link
Member

codyoss commented Jun 16, 2023

I believe this should be fine. This is used by a handful of our manual clients.

@noahdietz
Copy link
Contributor

Thanks @codyoss ! I've updated the PR in place. Setting automerge.

@noahdietz noahdietz added the automerge Merge the pull request once unit tests and other checks pass. label Jun 16, 2023
@noahdietz noahdietz removed the request for review from tritone June 16, 2023 20:35
tritone
tritone approved these changes Jun 17, 2023
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

I do feel like the contract is slightly ambiguous here, and it is a bit odd to have status.FromError return something for a non-gRPC error (even say if the call being wrapped for retry is not a gRPC call). However, I don't think this will cause any problems.

@tritone tritone added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 17, 2023
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 17, 2023
@gcf-merge-on-green gcf-merge-on-green bot merged commit 005d2df into googleapis:main Jun 17, 2023
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 17, 2023
@release-please release-please bot mentioned this pull request Jun 17, 2023
Merged
@atollena atollena deleted the issue-8127 branch June 18, 2023 09:07
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
@atollena
[internal/retry] Simplify gRPC status code mapping of retry error
13d38ec 
gRPC v1.55.0 that this project depends on came with a change in semantic for status.FromError such
that the gRPC status of wrapped errors is returned. The implementation of GRPCStatus in
internal/retry code was doing just that. Removing GRPCStatus entirely makes the code much easier to
understand: at the moment, status.FromError calls GRPCStatus which in turns calls status.FromError
and it's not immediately obviuos why this can't result in infinite recursion.

A previous change I made to this code made sure this method never returns
Status.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
@atollena
[internal/retry] Simplify gRPC status code mapping of retry error
800df30 
gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped
errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing
just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment,
status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately
obvious why this cannot result in infinite recursion.

A previous change I made to this code made sure this method never returns
status Code.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
@atollena atollena mentioned this pull request Jul 3, 2023
Merged
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
@atollena
(internal/retry) Simplify gRPC status code mapping of retry error
017c367 
gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped
errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing
just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment,
status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately
obvious why this cannot result in infinite recursion.

A previous change I made to this code made sure this method never returns
status Code.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
@atollena
fix(internal/retry): Simplify gRPC status code mapping of retry error
6a7ac3a 
gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped
errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing
just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment,
status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately
obvious why this cannot result in infinite recursion.

A previous change I made to this code made sure this method never returns
status Code.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noahdietz noahdietz noahdietz approved these changes

@tritone tritone tritone approved these changes

Assignees
No one assigned
Labels
size: s Pull request size is small.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@atollena @noahdietz @codyoss @tritone @kokoro-team