rpc_util: Reduce extra byte from limit reader #4957

uds5501 · 2021-11-05T17:28:33Z

RELEASE NOTES: None

uds5501 · 2021-11-05T17:31:24Z

Currently removing the +1 made sense to me for the problem.
Frankly, I couldn't understand the thought process of keeping it there in the first place, maybe someone could shed some light on the same?

dfawley

This is done to distinguish whether a message is exactly the same size as the limit or over the limit. Is there a test that covers a message of exactly the limit? I kind of doubt it, since I would have expected it to fail with this change. If not, please add one, thank you!

uds5501 · 2021-11-09T18:28:46Z

There is no such test I believe, will add one.
One small doubt though, how do I test decompress? Should I export it or copy the logic in the unit test?

dfawley · 2021-11-09T18:37:11Z

There is no such test I believe, will add one.
One small doubt though, how do I test decompress? Should I export it or copy the logic in the unit test?

This would be best as a test in grpc/test/ since those tests are done at the API layer. I wouldn't use proto messages for such a test (use the grpc APIs directly), so you can exactly control the payload size here. A decompressor can be implemented to do whatever you want it to do; the interface is small and pretty simple to implement. One example would be to output each byte in the input stream twice. Or, output the input contents and add one extra byte to the end.

uds5501 · 2021-11-11T15:33:20Z

This would be best as a test in grpc/test/ since those tests are done at the API layer. I wouldn't use proto messages for such a test (use the grpc APIs directly), so you can exactly control the payload size here. A decompressor can be implemented to do whatever you want it to do; the interface is small and pretty simple to implement. One example would be to output each byte in the input stream twice. Or, output the input contents and add one extra byte to the end.

Got it, I will try and add it by this weekend, a little caught up in official tasks.

uds5501 · 2021-11-14T06:48:46Z

Is there a test that covers a message of exactly the limit?

@dfawley I am able to implement the encoding.Compressor interface but I am not sure how do I write a test for this? How do I invoke the decompress() function in a way that data bytes == max receive limit?

Should I push the changes I have done so far?

dfawley · 2021-11-15T22:29:51Z

@uds5501 I would just have the compressor implement a Reader that outputs some number of bytes exactly equal to a magic value you initialize it with. And set the max receive message size on the client to the same value. E.g.

func (f *fakeCompressor) Decompress(r io.Reader) (io.Reader, error) {
	// Not sure if this needs to read from r at all.. if so, ioutil.ReadAll(r) and ignore the result.
	return bytes.NewBuffer(make([]byte, f.size)), nil 
}

uds5501 · 2021-11-16T17:48:14Z

@dfawley I have created a small sample test to see if the implementation is as expected.
If it is, please let me know, I will add the required comments on the sample interfaces afterward. Thank you.

dfawley

Thanks for writing this test. Comments inline.

dfawley · 2021-11-16T19:33:14Z

test/end2end_test.go

@@ -6730,6 +6730,73 @@ func testRPCTimeout(t *testing.T, e env) {
 	}
 }

+type FakeCompressor struct {


No need to export. It would be nice to parameterize the size (or add a payload buffer directly here?) for reusability.

dfawley · 2021-11-16T19:35:58Z

test/end2end_test.go

+}
+
+func (c *FakeCompressor) Compress(w io.Writer) (io.WriteCloser, error) {
+	customCloser := &customWriterCloser{w}


Nit: use field names: Writer: w. Also, omit local variable and return directly.

dfawley · 2021-11-16T19:37:03Z

test/end2end_test.go

@@ -6730,6 +6730,73 @@ func testRPCTimeout(t *testing.T, e env) {
 	}
 }

+type FakeCompressor struct {
+	name string


This field is unnecessary, or should be returned from Name().

dfawley · 2021-11-16T19:37:41Z

test/end2end_test.go

+
+func (s) TestDecompressionWithMaximumBytes(t *testing.T) {
+	encoding.RegisterCompressor(&FakeCompressor{name: "fakeCompressor"})
+	payload, err := newPayload(testpb.PayloadType_COMPRESSABLE, 10098)


This payload is actually unnecessary.

dfawley · 2021-11-16T19:38:16Z

test/end2end_test.go

+}
+
+func (c *FakeCompressor) Decompress(r io.Reader) (io.Reader, error) {
+	return bytes.NewBuffer(make([]byte, 10098)), nil


This will decompress zeroes -- will it work with protobuf to deserialize this?

I am not sure actually, i couldn't study the test server well enough. I went through a few examples used in tests so copied one of them, I'll admit that I don't see how decompression will work here. Will research more on this.

dfawley · 2021-11-16T19:39:42Z

test/end2end_test.go

+	s := grpc.NewServer()
+	testpb.RegisterTestServiceServer(s, ss)
+	lis, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatalf("Failed to create listener: %v", err)
+	}
+
+	go func() {
+		s.Serve(lis)
+	}()
+	defer s.Stop()
+
+	dctx, dcancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer dcancel()
+	cc, err := grpc.DialContext(dctx, lis.Addr().String(), grpc.WithInsecure(), grpc.WithDefaultCallOptions(grpc.UseCompressor("fakeCompressor"), grpc.MaxCallRecvMsgSize(10098)))
+	if err != nil {
+		t.Fatalf("Failed to dial server")
+	}
+	defer cc.Close()


Since you are using protobuf..assuming it works..why not use ss.Start(nil, <dial options>)?

Again, not sure.
Will need some guidance to understand where is our decompression actually coming into play.

dfawley · 2021-11-16T19:40:40Z

test/end2end_test.go

+
+	dctx, dcancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer dcancel()
+	cc, err := grpc.DialContext(dctx, lis.Addr().String(), grpc.WithInsecure(), grpc.WithDefaultCallOptions(grpc.UseCompressor("fakeCompressor"), grpc.MaxCallRecvMsgSize(10098)))


We also need to test with a max recv msg size of N-1 and ensure it fails. This is where I believe your PR is incorrect.

easwars · 2021-11-17T17:39:24Z

@uds5501 : Looks like the tests are broken.

github-actions · 2021-11-25T18:48:15Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

rpc_util: Reduce extra byte from limit reader

917ffd7

zasweq added the Type: Bug label Nov 6, 2021

zasweq added this to the 1.43 release milestone Nov 6, 2021

zasweq requested a review from dfawley November 6, 2021 00:04

zasweq assigned dfawley Nov 6, 2021

dfawley requested changes Nov 8, 2021

View reviewed changes

dfawley assigned uds5501 and unassigned dfawley Nov 8, 2021

dfawley added the Status: Requires Reporter Clarification label Nov 10, 2021

compressor: Implement compressor

3a9ea2a

test: Test compression for payload with max recv size

c648182

uds5501 force-pushed the decompressor-fix branch from 798f00c to c648182 Compare November 16, 2021 17:46

uds5501 requested a review from dfawley November 16, 2021 17:48

dfawley reviewed Nov 16, 2021

View reviewed changes

github-actions bot added the stale label Nov 25, 2021

github-actions bot closed this Dec 2, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc_util: Reduce extra byte from limit reader #4957

rpc_util: Reduce extra byte from limit reader #4957

uds5501 commented Nov 5, 2021 •

edited by zasweq

uds5501 commented Nov 5, 2021

dfawley left a comment

uds5501 commented Nov 9, 2021

dfawley commented Nov 9, 2021

uds5501 commented Nov 11, 2021

uds5501 commented Nov 14, 2021

dfawley commented Nov 15, 2021

uds5501 commented Nov 16, 2021

dfawley left a comment

dfawley Nov 16, 2021

dfawley Nov 16, 2021

dfawley Nov 16, 2021

dfawley Nov 16, 2021

dfawley Nov 16, 2021

uds5501 Nov 19, 2021

dfawley Nov 16, 2021

uds5501 Nov 19, 2021

dfawley Nov 16, 2021

easwars commented Nov 17, 2021

github-actions bot commented Nov 25, 2021

rpc_util: Reduce extra byte from limit reader #4957

rpc_util: Reduce extra byte from limit reader #4957

Conversation

uds5501 commented Nov 5, 2021 • edited by zasweq

uds5501 commented Nov 5, 2021

dfawley left a comment

Choose a reason for hiding this comment

uds5501 commented Nov 9, 2021

dfawley commented Nov 9, 2021

uds5501 commented Nov 11, 2021

uds5501 commented Nov 14, 2021

dfawley commented Nov 15, 2021

uds5501 commented Nov 16, 2021

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Nov 17, 2021

github-actions bot commented Nov 25, 2021

uds5501 commented Nov 5, 2021 •

edited by zasweq