Skip to content
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

Create tests for two potential cardinality violation bugs #7570

Open
dfawley opened this issue Aug 29, 2024 · 2 comments
Open

Create tests for two potential cardinality violation bugs #7570

dfawley opened this issue Aug 29, 2024 · 2 comments
Labels
Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. Status: Help Wanted Type: Testing

Comments

@dfawley
Copy link
Member

dfawley commented Aug 29, 2024

When looking into the generic stream interface documentation, I noticed a couple potential bugs in our code through inspection:

  1. Client should ensure a cardinality violation error is returned for client-streaming RPCs if the server doesn't send a message before returning status OK.

Test case: Using stubserver, create a client-streaming RPC handler (will need to be added similar to the unary/bidi streaming handlers) that just does return nil without first calling SendAndClose. This should result in the client receiving an error when making this call and calling CloseAndRecv. Also in this case, we should receive Error log messages indicating what happened and that it is illegal behavior. Client streaming RPCs must terminate with either no message and a non-OK status, or a message and an OK status.

We can also test the reverse case where it calls SendAndClose and returns an error from the handler that results in an RPC status. This is not necessarily "wrong" since SendAndClose itself could return an error, but the behavior of the RPC should be that the RPC is terminated when SendAndClose is called, so the return value of the handler should be ignored.

  1. Also for client-streaming RPCs, when SendAndClose is called, the server should perform a RST_STREAM() after sending the response message successfully. We should confirm this is happening.

Test case: Using stubserver, create a client-streaming RPC handler (like above) that calls SendAndClose and then calls Recv. Those operations should fail with an error indicating the stream was already closed. On the client side, just send messages continuously. An error should eventually propagate to the client's Send calls.

Implementation plan:

Check if similar tests exist first. Then create these tests if not. If they fail, let's t.Skip them and check them in, and then file issues and fix the bugs and re-enable the tests.

@dfawley dfawley added Status: Help Wanted Type: Testing Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. labels Aug 29, 2024
@dfawley
Copy link
Member Author

dfawley commented Sep 11, 2024

(2) is actually tricky to fix, because there is a behavior change that is required:

func ClientStreamingMethodHandler(stream ___) error {
	// ...
	stream.SendAndClose(responseMessage)
	stream.SetTrailer(...)
	return nil
}

This used to work (trailers in SetTrailer are sent upon return), but wouldn't work if we make SendAndClose send trailers and RST_STREAM.

Maybe there's another way to prevent Recv from continuing to work after SendAndClose is called.

@dfawley
Copy link
Member Author

dfawley commented Oct 8, 2024

Somewhat related to #7286

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. Status: Help Wanted Type: Testing
Projects
None yet
Development

No branches or pull requests

1 participant