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

status: support wrapped errors in FromContextError #4977

Merged
merged 1 commit into from Nov 19, 2021

Conversation

bestbeforetoday
Copy link
Contributor

@bestbeforetoday bestbeforetoday commented Nov 12, 2021

Return an appropriate Status from status.FromContext error if either the supplied error or an error in its chain is one of the context sentinel error values.

Closes #4976

RELEASE NOTES:

  • status: support wrapped errors in FromContextError

Return an appropriate Status from status.FromContext error if either the supplied error or an error in its chain is one of the context sentinel error values.
@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Nov 12, 2021

CLA Signed

The committers are authorized under a signed CLA.

@dfawley
Copy link
Contributor

@dfawley dfawley commented Nov 12, 2021

Is there a reason this was created as a draft PR?

Please agree to the CLA or we cannot take this PR.

Otherwise this looks good to me. There is a chance we need to re-add support for Go1.11 in the future, but if we do that we can have a 1.11 implementation of this function that does not use errors.Is.

@bestbeforetoday
Copy link
Contributor Author

@bestbeforetoday bestbeforetoday commented Nov 12, 2021

Thank you for the quick feedback. I created the PR as a draft:

  1. so you could take as look at a suggested implementation; and
  2. although I already contribute to several Linux Foundation projects, I need to check with my employer before signing up to the EasyCLA, either as an individual or under an organization agreement. I'll do that in the coming week.

@bestbeforetoday bestbeforetoday marked this pull request as ready for review Nov 16, 2021
@easwars easwars added this to the 1.43 release milestone Nov 17, 2021
@easwars easwars added Type: Behavior Change and removed Status: Requires Reporter Clarification labels Nov 17, 2021
@easwars easwars assigned dfawley and easwars and unassigned bestbeforetoday Nov 17, 2021
@easwars easwars requested a review from dfawley Nov 19, 2021
@dfawley dfawley changed the title Support wrapped errors in status.FromContextError status: support wrapped errors in FromContextError Nov 19, 2021
@dfawley dfawley merged commit d542bfc into grpc:master Nov 19, 2021
10 checks passed
andreimatei added a commit to andreimatei/grpc-go-1 that referenced this issue Jan 7, 2022
pickerWrapper had logic very similar to status.FromContextError() for
transforming Context errors to status errors. This patch removes the
duplication by delegating to the status library. Besides removing the
code duplication, the status library is arguably more robust because it
doesn't rely on ctx.Error() to only ever return two types of errors.

I believe this patch and the previous one stand on their own, but, FWIW,
they're also motivating by me wanting to experiment in the CockroachDB
codebase with using a custom implementation of context.Context whose
Err() method can return better errors than the stdlib context.Context.
These errors would still wrap context.Canceled.  Such an implementation
would technically break the documentation of context.Context, which
seems to exhaustively list the sentinel error that context.Context can
return. Still, as grpc#4977 showed, most
code should support wrapped context errors. This patch moves from "most
code" to "all code" in gRPC. I haven't checked which of the callsites
I've touched use contexts that might be inherited from a gRPC client, as
opposed to contexts derived inside gRPC from context.Background (which
contexts would not be affected by whatever I do outside of gRPC), but
unifying all the context error handling code seems like a good idea to
me universally.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Behavior Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants