Skip to content

internal/transport: remove duplicate code handling ctx errors#5112

Closed
andreimatei wants to merge 2 commits intogrpc:masterfrom
andreimatei:ctx-err
Closed

internal/transport: remove duplicate code handling ctx errors#5112
andreimatei wants to merge 2 commits intogrpc:masterfrom
andreimatei:ctx-err

Conversation

@andreimatei
Copy link
Copy Markdown

@andreimatei andreimatei commented Jan 7, 2022

See individual commits.

RELEASE NOTES: none

transport.ContextErr() was very similar to status.FromContextError().
Besides the code duplication, the latter is arguably better because it
handles errors wrapping context errors, and the former only supports raw
context errors. This patch does away with transport.ContextErr().
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.
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Jan 10, 2022
@easwars easwars added this to the 1.44 Release milestone Jan 10, 2022
@zasweq zasweq modified the milestones: 1.44 Release, 1.45 Release Jan 11, 2022
@zasweq zasweq self-assigned this Jan 12, 2022
Copy link
Copy Markdown
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

Hello, this is an API change, and is externally facing and exported. Adding a parameter is not backwards compatible, and so unfortunately we cannot take this change. Feel free to make try and API change in internal/

@zasweq zasweq closed this Jan 19, 2022
@andreimatei
Copy link
Copy Markdown
Author

Would you take everything except the change to status. FromContextError() ?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants