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 bot commented Nov 12, 2021

CLA Signed

The committers are authorized under a signed CLA.

@dfawley
Copy link
Member

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 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 November 16, 2021 13:29
@easwars easwars added this to the 1.43 release milestone Nov 17, 2021
@easwars easwars added Type: Behavior Change Behavior changes not categorized as bugs 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 November 19, 2021 00:23
@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
andreimatei added a commit to andreimatei/grpc-go-1 that referenced this pull request 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 Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow status.FromContextError to work with wrapped errors
3 participants