Skip to content

Cardinality violations in Client streaming and Unary RPC. #8330

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pranjali-2501
Copy link
Contributor

Client should ensure an INTERNAL error is returned for non-server streaming RPCs if the server attempts to send more than one response.
Currently it is returning UNKNOWN, which should not be the case for cardinality violations.

Filed in #7286
RELEASE NOTES: N/A

Copy link

codecov bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.15%. Comparing base (6821606) to head (5da4f32).
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8330      +/-   ##
==========================================
- Coverage   82.25%   82.15%   -0.11%     
==========================================
  Files         417      419       +2     
  Lines       41356    42025     +669     
==========================================
+ Hits        34017    34524     +507     
- Misses       5923     6031     +108     
- Partials     1416     1470      +54     
Files with missing lines Coverage Δ
stream.go 81.65% <100.00%> (+0.10%) ⬆️

... and 62 files with indirect coverage changes

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

@Pranjali-2501 Pranjali-2501 added this to the 1.73 Release milestone May 15, 2025
@Pranjali-2501 Pranjali-2501 self-assigned this May 15, 2025
@dfawley
Copy link
Member

dfawley commented May 16, 2025

@Pranjali-2501 please use the Assignees field to indicate who needs to act on the PR next. I assume you have meant to assign this to @arjan-bal and me?

@dfawley dfawley assigned dfawley and arjan-bal and unassigned Pranjali-2501 May 16, 2025
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple really small things. Thanks!

@@ -3739,6 +3739,39 @@ func (s) TestClientStreaming_ReturnErrorAfterSendAndClose(t *testing.T) {
}
}

// Tests that a client receives a cardinality violation error for client-streaming
// RPCs if the server call SendAndClose after calling SendAndClose.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// RPCs if the server call SendAndClose after calling SendAndClose.
// RPCs if the server calls SendAndClose multiple times.

t.Fatalf("stream.Send(_) = %v, want <nil>", err)
}
if _, err := stream.CloseAndRecv(); status.Code(err) != codes.Internal {
t.Fatalf("stream.CloseAndRecv() = %v, want error %s", err, codes.Internal)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
t.Fatalf("stream.CloseAndRecv() = %v, want error %s", err, codes.Internal)
t.Fatalf("stream.CloseAndRecv() = %v, want error with status code %s", err, codes.Internal)

@dfawley dfawley assigned Pranjali-2501 and unassigned dfawley May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants