Skip to content

Conversation

costela
Copy link

@costela costela commented Apr 10, 2025

Since #6862 was closed as wontfix, this is a follow-up proposal to actuall pass along errors the caller may be interested it.

The current wrapped errors are only used for the specific cases of context cancellation and deadline. The rationale being: these are the errors that are caused via passed in context arguments from the caller.

Copy link

linux-foundation-easycla bot commented Apr 10, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: costela / name: Leo Antunes (0233fb6)

Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.19%. Comparing base (732f3f3) to head (0233fb6).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
internal/status/status.go 70.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8241      +/-   ##
==========================================
+ Coverage   82.09%   82.19%   +0.09%     
==========================================
  Files         412      412              
  Lines       40491    40503      +12     
==========================================
+ Hits        33242    33291      +49     
+ Misses       5876     5848      -28     
+ Partials     1373     1364       -9     
Files with missing lines Coverage Δ
rpc_util.go 82.24% <ø> (ø)
status/status.go 100.00% <100.00%> (ø)
internal/status/status.go 88.57% <70.00%> (-1.96%) ⬇️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Since grpc#6862 was closed as wontfix, this is a follow-up proposal to actuall pass along errors the caller may be interested it.

The current wrapped errors are only used for the specific cases of context cancellation and deadline. The rationale being: these are the errors that are caused via passed in context arguments from the caller.
@costela costela force-pushed the wrap-context-errors-to-satisfy-errors.Is branch from c0a3007 to 0233fb6 Compare April 10, 2025 09:41
@costela costela marked this pull request as ready for review April 10, 2025 13:50
@arjan-bal arjan-bal self-assigned this Apr 14, 2025
@arjan-bal arjan-bal self-requested a review April 14, 2025 03:37
@arjan-bal
Copy link
Contributor

There were 3 issues mentioned in the previously closed issue: #6862 (comment)

  1. I think it would not be appropriate to wrap context.Canceled/DeadlineExceeded if the server returns those statuses, because the context itself may still be valid.
  2. We could possibly (? -it might be easy, or it might be very very hard) detect whether the cancelation/deadline happened on the client-side and only conditionally wrap.
  3. If we did 2, it's still theoretically possible there could be a race between the server deciding the client's deadline was exceeded and the client noticing the context expired, meaning we could get it wrong sometimes.

For 1 & 2, the error types modified in the PR are only used for client side context cancellation, so we should be good. For 3, the client would still see status DEADLINE_EXCEEDED with the message set here, which would probably give a nil error on unwrapping:

if statusCode == codes.Canceled {
if d, ok := s.ctx.Deadline(); ok && !d.After(time.Now()) {
// Our deadline was already exceeded, and that was likely the cause
// of this cancellation. Alter the status code accordingly.
statusCode = codes.DeadlineExceeded
}
}
t.closeStream(s, io.EOF, false, http2.ErrCodeNo, status.Newf(statusCode, "stream terminated by RST_STREAM with error code: %v", f.ErrCode), nil, false)

@dfawley would like to get your thoughts on this since you handled #6862.

@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Apr 14, 2025
@costela
Copy link
Author

costela commented Apr 14, 2025

@arjan-bal thanks for the summary! Yes, the intention was to side-step the issues mentioned in the previous PR.

@dfawley
Copy link
Member

dfawley commented Apr 16, 2025

It would be better if you could follow the standard feature request issue template instead of creating a PR (https://github.com/grpc/grpc-go/issues/new?template=feature.md). Specifically, what problem are you attempting to solve by this?

@dfawley dfawley closed this Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants