client: wrap context errors returned by DialContext#8835
client: wrap context errors returned by DialContext#8835easwars wants to merge 1 commit intogrpc:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8835 +/- ##
==========================================
- Coverage 83.31% 83.23% -0.08%
==========================================
Files 414 414
Lines 32753 32753
==========================================
- Hits 27288 27263 -25
- Misses 4064 4083 +19
- Partials 1401 1407 +6
🚀 New features to boost your workflow:
|
| conn, err = nil, ctx.Err() | ||
| default: | ||
| conn, err = nil, fmt.Errorf("%v: %v", ctx.Err(), err) | ||
| conn, err = nil, fmt.Errorf("%v: %w", err, ctx.Err()) |
There was a problem hiding this comment.
Because we are returning wrapped errors, the inner error has become part of our public API. We must avoid breaking users who rely on errors.Is in the future. I think we should ensure all possible wrapped errors are explicitly documented in the godoc.
See go/go-style/best-practices#documentation-conventions-errors
| conn, err = nil, ctx.Err() | ||
| default: | ||
| conn, err = nil, fmt.Errorf("%v: %v", ctx.Err(), err) | ||
| conn, err = nil, fmt.Errorf("%v: %w", err, ctx.Err()) |
There was a problem hiding this comment.
Why reverse the order of the text output?
Also can we switch this outer select to:
ctxErr := ctx.Err()
if ctxErr == nil {
return // use err as-is
}
switch { ..... }But then that raises a question for me: if we aren't doing returnLastError, but FailOnNonTempDialError is set, what does that even mean? In this case, it looks like we are returning the connection error, unless there is also a context error, in which case we return it instead?
Should FailOnNonTempDialError imply both WithBlock and WithReturnConnectionError? I guess it shouldn't imply WithBlock but it seems like it should set returnLastError otherwise it's unpredictable.
5 participants
Fixes #4822
RELEASE NOTES:
DialContextnow wraps context errors, allowing users to useerrors.Isto check for timeouts or cancellations