transport: add status details even when aborting early#8754
transport: add status details even when aborting early#8754arjan-bal merged 3 commits intogrpc:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8754 +/- ##
==========================================
- Coverage 83.35% 83.14% -0.22%
==========================================
Files 418 418
Lines 32377 32513 +136
==========================================
+ Hits 26989 27033 +44
+ Misses 4022 4014 -8
- Partials 1366 1466 +100
🚀 New features to boost your workflow:
|
9bd760d to
10252fe
Compare
revive errs
10252fe to
1b5a905
Compare
arjan-bal
left a comment
There was a problem hiding this comment.
LGTM, adding a second reviewer. Please address the minor comments.
test/end2end_test.go
Outdated
| st := status.New(codes.ResourceExhausted, "rate limit exceeded") | ||
| st, err := st.WithDetails(wantDetails) | ||
| if err != nil { | ||
| t.Fatalf("status.WithDetails() failed: %v", err) |
There was a problem hiding this comment.
Nit: All these could be moved to be inside the tapHandler.
There was a problem hiding this comment.
Each stream is handled in a separate goroutine on the server. We should not be calling t.Fatalf outside the main test function, see https://google.github.io/styleguide/go/best-practices#dont-call-tfatal-from-separate-goroutines.
We should either use t.Errorf and return an error, or revert back to creating the status object in the main test function.
There was a problem hiding this comment.
I agree that we should not be calling t.Fatalf from outside the main test goroutine. But we don't expect this ever fail. We've handcrafted the status right here, and WithDetails fails only when the code is OK or if the struct can't be marshaled into an Any.
I'm Ok with changing it to t.Errorf or switching it back to how it was earlier.
8b58cf2 to
2c329c8
Compare
Modifies
earlyAbortStreamHandlerto include status details if present.Most use cases of
earlyAbortStreamHandlerare for circumstances where there are certainly no error details (bad HTTP methods, bad content-type, internal error, etc). However, tap handlers also typically go through theearlyAbortStreamHandler. Inhttp2_server.go:Yet the handler does not include error details by default, limiting how tap handlers can be used and breaking some user assumptions surrounding which information is propagated.
This PR fixes this by checking for status details and including the header for them if present.
RELEASE NOTES: