grpc: fix receiving empty messages when compression is enabled and maxReceiveMessageSize is maxInt64 #7753

misvivek · 2024-10-17T05:44:57Z

Fixes #4552

RELEASE NOTES:

grpc: fix receiving empty messages when compression is enabled and maxReceiveMessageSize is maxInt64

linux-foundation-easycla · 2024-10-17T05:45:01Z

The committers listed above are authorized under a signed CLA.

✅ login: misvivek (32d1ca1, 786724d, eb85e40, 2bf22b3, c830a6c, 94f6a63, fad8892, 5bbf7d6, 6dfb4a9, 8f51348, 63934e6, b1bd3b3, 56f4538, f3afb78)

purnesh42H · 2024-10-17T06:44:50Z

@misvivek please keep the description and pr title in the format as I have updated

misvivek · 2024-10-17T07:09:45Z

noted @purnesh42H

rpc_util.go

purnesh42H · 2024-10-17T07:27:43Z

rpc_util.go

+	if err = checkReceiveMessageOverflow(int64(out.Len()), int64(maxReceiveMessageSize), dcReader); err != nil {
+		return nil, out.Len() + 1, err
+	}
+	return out, len(out), err


return out.Len()

purnesh42H · 2024-10-17T07:32:17Z

rpc_util.go

+	if readBytes == maxReceiveMessageSize {
+		b := make([]byte, 1)
+		if n, err := dcReader.Read(b); n > 0 || err != io.EOF {
+			return fmt.Errorf("overflow: message larger than max size receivable by client (%d bytes)", maxReceiveMessageSize)


"overflow: received message size is larger than the allowed maxReceiveMessageSize (%d bytes)."

purnesh42H · 2024-10-17T07:32:47Z

rpc_util_test.go

@@ -290,3 +292,130 @@ func BenchmarkGZIPCompressor512KiB(b *testing.B) {
 func BenchmarkGZIPCompressor1MiB(b *testing.B) {
 	bmCompressor(b, 1024*1024, NewGZIPCompressor())
 }
+
+type mockCompressor struct {


s/mockCompressor/testCompressor

purnesh42H · 2024-10-17T07:33:50Z

rpc_util_test.go

@@ -290,3 +292,130 @@ func BenchmarkGZIPCompressor512KiB(b *testing.B) {
 func BenchmarkGZIPCompressor1MiB(b *testing.B) {
 	bmCompressor(b, 1024*1024, NewGZIPCompressor())
 }
+
+type mockCompressor struct {


top level docstring explaining the use of this testCompressor and top level comments for all the functions

purnesh42H · 2024-10-17T07:36:51Z

rpc_util_test.go

+				t.Errorf("decompress() size = %d, want %d", size, tt.size)
+			}
+			if len(tt.want) != len(output) {
+				t.Errorf("decompress() output length = %d, want %d", output, tt.want)


decompress() output length, got = %d, want = %d

purnesh42H · 2024-10-17T07:38:39Z

rpc_util_test.go

+			name: "Successful decompression",
+			compressor: &mockCompressor{
+				DecompressedData: []byte("decompressed data"),
+				Size:             17,


why do you need "Size" as different attribute, can't you calculate from DecompressedData?

purnesh42H · 2024-10-17T07:39:22Z

rpc_util_test.go

@@ -290,3 +292,130 @@ func BenchmarkGZIPCompressor512KiB(b *testing.B) {
 func BenchmarkGZIPCompressor1MiB(b *testing.B) {
 	bmCompressor(b, 1024*1024, NewGZIPCompressor())
 }
+
+type mockCompressor struct {
+	DecompressedData []byte


why are these attributes upper case? We are not exporting them

changed to lowercase

purnesh42H · 2024-10-17T07:45:47Z

rpc_util_test.go

+		input                 mem.BufferSlice
+		maxReceiveMessageSize int
+		want                  mem.BufferSlice
+		size                  int


what is this size for? Please give more meaningful name

purnesh42H · 2024-10-17T07:48:18Z

rpc_util_test.go

+	}{
+		{
+			name: "Successful decompression",
+			compressor: &mockCompressor{


do we really need to test compressor for all cases? Can't we just use a regular compressor?

Part of this test is to also verify that inputs work with regular compressor

Something like this @purnesh42H

``compressor := gzip.NewCompressor()
originalData := []byte("test data")

// Compress compressedData, err := compressor.Compress(originalData) if err != nil { t.Fatalf("Failed to compress data: %v", err) } // Decompress decompressedData, err := compressor.Decompress(compressedData) if err != nil { t.Fatalf("Failed to decompress data: %v", err) } if !bytes.Equal(decompressedData, originalData) { t.Errorf("Decompressed data does not match original") }``

yeah i think we should try to get the regular test cases working with regular compressor. Only use testCompressor when we want to simulate or enforce an error

can you check this and see how are different compressors are tested https://github.com/grpc/grpc-go/blob/56df169480cdb4928a24a50b5289f909f0d81ba7/test/compressor_test.go ?

We should try to do something similar. There are test compressors as well regular compressors based on test cases. Compressor can be part of test case struct

One more example

grpc-go/rpc_util_test.go

Line 151 in 56df169

func (s) TestCompress(t *testing.T) {

purnesh42H

Let's try to make as many as tests as we can using regular compressor and then we can review the rest

purnesh42H · 2024-10-18T06:18:41Z

rpc_util_test.go

+// used for testing compression and decompression functionality.
+// Compress is a placeholder for the compression logic.
+// It currently returns a nil WriteCloser and no error, as the actual compression is not implemented.
+// DecompressedSize returns the pre-configured size of the decompressed data.


these comments should be against the attributes of struct, not in docstring

purnesh42H · 2024-10-18T06:20:40Z

rpc_util_test.go

+	}{
+		{
+			name: "Successful decompression",
+			compressor: &mockCompressor{


yeah i think we should try to get the regular test cases working with regular compressor. Only use testCompressor when we want to simulate or enforce an error

codecov · 2024-10-22T06:20:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.98%. Comparing base (c63aeef) to head (f3afb78).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7753      +/-   ##
==========================================
+ Coverage   81.80%   81.98%   +0.17%     
==========================================
  Files         375      377       +2     
  Lines       37978    38132     +154     
==========================================
+ Hits        31068    31262     +194     
+ Misses       5609     5564      -45     
- Partials     1301     1306       +5

Files with missing lines	Coverage Δ
rpc_util.go	`79.94% <100.00%> (+0.62%)`	⬆️

... and 33 files with indirect coverage changes

purnesh42H · 2024-10-29T05:23:24Z

rpc_util_test.go

+		t.Fatalf("Could not initialize gzip compressor with level 5 compression.")
+	}
+
+	for _, test := range []struct {


@misvivek i don't understand what this test loop is doing and what is it testing. This seems totally unnecessary for the change (line 349-373)

purnesh42H · 2024-10-29T05:24:02Z

rpc_util_test.go

+	}{
+		{
+			name: "Successful decompression",
+			compressor: &testCompressor{


you have to use real compressor here. I have mentioned this so many times. Why do you keep using test compressor here?

purnesh42H · 2024-11-07T09:05:42Z

rpc_util_test.go

@@ -290,3 +293,129 @@ func BenchmarkGZIPCompressor512KiB(b *testing.B) {
 func BenchmarkGZIPCompressor1MiB(b *testing.B) {
 	bmCompressor(b, 1024*1024, NewGZIPCompressor())
 }
+
+type testCompressor struct {


this testCompressor should not be used ideally. If at all we are using it, we should not use it for test cases involving max receive size and actual receive message size

purnesh42H · 2024-11-07T09:15:52Z

rpc_util_test.go

+		error                 error
+	}{
+		{
+			name:       "Successful decompression",


add more context to name. For example, in this case actual message size is less than allowed receive size. Keep it short though.

purnesh42H · 2024-11-07T09:16:17Z

rpc_util_test.go

+			error:          nil,
+		},
+		{
+			name:                  "Error during decompression",


same here. name should be meaningful in terms of input and output

purnesh42H · 2024-11-07T09:16:26Z

rpc_util_test.go

+			error:                 errors.New("decompression error"),
+		},
+		{
+			name: "Buffer overflow",


this is a better name

purnesh42H · 2024-11-07T09:16:40Z

rpc_util_test.go

+		},
+		{
+			name: "Buffer overflow",
+			compressor: &testCompressor{


testCompressor should not be required here

purnesh42H · 2024-11-07T09:17:58Z

rpc_util_test.go

+		},
+		{
+			name: "MaxInt64 receive size with small data",
+			compressor: &testCompressor{


again, testCompressor should not be required here. We should use a regular compressor and simulate a scenario where receiveMaxSize is Int64 and actual data is small and that should pass.

purnesh42H · 2024-11-08T09:39:35Z

rpc_util_test.go

+		error                 error
+	}{
+		{
+			name:       "Decompresses successfully with sufficient buffer size",


success, receive message within maxReceiveMessageSize

purnesh42H · 2024-11-08T09:40:52Z

rpc_util_test.go

+			error:                 errors.New("decompression error"),
+		},
+		{
+			name:       "Fails with overflow error when decompressed size exceeds maxReceiveMessageSize",


overflow failure, receive message exceeds maxReceiveMessageSize

purnesh42H · 2024-11-08T09:42:25Z

rpc_util_test.go

+			error:          nil,
+		},
+		{
+			name:                  "Fails with io.Copy error during decompression",


what is this test case? Do we need it?

done removed

purnesh42H · 2024-11-08T09:43:34Z

rpc_util_test.go

+			error:                 errors.New("overflow: received message size is larger than the allowed maxReceiveMessageSize (5 bytes)."),
+		},
+		{
+			name:       "Returns EOF error when maxReceiveMessageSize is MaxInt64 but data ends prematurely",


EOF is not really failure. This test case is similar to case 1) except maxReceiveMessageSize is math.MAXInt64

purnesh42H · 2024-11-08T09:44:13Z

rpc_util_test.go

+			compressedSize:        0,
+			error:                 errors.New("simulated io.Copy read error"),
+		},
+		{


this is redundant. Case 1) already test it?

done remove

purnesh42H · 2024-11-08T09:50:45Z

rpc_util_test.go

+		{
+			name:       "Decompresses successfully with sufficient buffer size",
+			compressor: c,
+			input: func() mem.BufferSlice {


As suggested, change this to []byte and do mem.BufferSlice when calling decompress

purnesh42H · 2024-11-08T09:52:01Z

rpc_util_test.go

+				return mem.BufferSlice{mem.NewBuffer(&compressedData, nil)}
+			}(),
+			maxReceiveMessageSize: 100,
+			want: func() mem.BufferSlice {


same here. this should be just []byte. When comparing you should compare to this in []byte format

purnesh42H · 2024-11-08T09:52:48Z

rpc_util_test.go

+			if size != tt.compressedSize {
+				t.Errorf("decompress() size, got = %d, want = %d", size, tt.compressedSize)
+			}
+			if len(tt.want) != len(output) {


you should compare the actual bytes instead of just comparing the length.

purnesh42H · 2024-11-08T09:53:43Z

rpc_util_test.go

+			output, size, err := decompress(tt.compressor, tt.input, tt.maxReceiveMessageSize, nil)
+			// Check if both err and tt.error are either nil or non-nil
+			if (err != nil) != (tt.error != nil) {
+				t.Errorf("decompress() error, got err=%v, want err=%v", err, tt.error)


all should change to t.Fatalf because we don't want to continue once detecting an error. Like you won't compare compressed size if decompression has returned error

purnesh42H · 2024-11-08T09:54:33Z

rpc_util_test.go

+		input                 mem.BufferSlice
+		maxReceiveMessageSize int
+		want                  mem.BufferSlice
+		compressedSize        int


why do you need compressedSize as parameter here. It can be easily calculated from compressed data you are providing.

compressedSize := 0 for _, buf := range tt.input { compressedSize += len(buf.Bytes()) } your are suggesting like this

purnesh42H · 2024-11-20T05:30:26Z

rpc_util.go

@@ -900,12 +899,33 @@ func decompress(compressor encoding.Compressor, d mem.BufferSlice, maxReceiveMes
 	//}

 	var out mem.BufferSlice
-	_, err = io.Copy(mem.NewWriter(&out, pool), io.LimitReader(dcReader, int64(maxReceiveMessageSize)+1))
+	_, err = io.Copy(mem.NewWriter(&out, pool), io.LimitReader(dcReader, int64(maxReceiveMessageSize)))


@misvivek can you rebase with latest main branch. This has changed to mem.ReadAll. Can you verify if bug still exist?

Please verify if the bug still exist first with me.ReadAll. Don't change that

purnesh42H · 2024-11-20T05:31:29Z

rpc_util_test.go

+			error:                 errors.New("overflow: received message size is larger than the allowed maxReceiveMessageSize (5 bytes)"),
+		},
+		{
+			name:                  "Succeeds when message size is much smaller than maxReceiveMessageSize",


this is similar to case 1. We don't need it

purnesh42H · 2024-11-20T05:33:38Z

rpc_util_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			message := func() mem.BufferSlice {


this is a strange way of initializing. It should be done something like below

mem.BufferSlice{mem.NewBuffer(&input, nil)}

we have to send the compressed message, so we are doing the above way,
we tried your suggestion then we are getting like gzip: invalid header,

purnesh42H · 2024-11-20T05:34:04Z

rpc_util_test.go

+				return mem.BufferSlice{mem.NewBuffer(&compressedData, nil)}
+			}()
+			output, size, err := decompress(tt.compressor, message, tt.maxReceiveMessageSize, nil)
+			wantMsg := func() mem.BufferSlice {


same here. Initialize as suggested above.

purnesh42H · 2024-11-20T05:34:38Z

rpc_util_test.go

+				decompressed := tt.want
+				return mem.BufferSlice{mem.NewBuffer(&decompressed, nil)}
+			}()
+


no need of new lines when checking different things. Remove them please.

purnesh42H · 2024-11-20T05:35:53Z

rpc_util_test.go

+		},
+	}
+
+	for _, tt := range tests {


purnesh42H · 2024-11-20T05:36:52Z

rpc_util_test.go

+			}()
+
+			// Check for expected error
+			if (err != nil) != (tt.error != nil) {


it should be more clear. Something like if test.error != nil && err == nil then throw error that we got nil error when we want non-nil err

purnesh42H · 2024-11-20T05:38:32Z

rpc_util_test.go

+
+			}
+			// to check when we are passing empty bytes output is "nil"  failure, empty receive message case
+			if wantMsg != nil && output != nil {


there should be separate condition to check if wanMsg is non-nil and output is nil. Then it should throw error that got nil when we want non-nil

purnesh42H · 2024-11-21T06:25:11Z

rpc_util_test.go

+			if wantMsg != nil && output == nil {
+				t.Fatalf("decompress() got = nil, want = non nil")
+			}
+			if wantMsg != nil && output != nil {


no, need of checking non nil again for output as you already checked that

purnesh42H changed the title ~~4552 empty response on enable decompression~~ grpc: fix getting empty response when compress enabled and maxReceiveMessageSize be maxInt64 Oct 17, 2024

purnesh42H reviewed Oct 17, 2024

View reviewed changes

purnesh42H assigned misvivek Oct 17, 2024

purnesh42H added the Type: Bug label Oct 17, 2024

purnesh42H added this to the 1.69 Release milestone Oct 17, 2024

misvivek requested a review from purnesh42H October 17, 2024 18:28

purnesh42H reviewed Oct 18, 2024

View reviewed changes

purnesh42H reviewed Oct 29, 2024

View reviewed changes

misvivek requested a review from purnesh42H November 7, 2024 04:05

purnesh42H reviewed Nov 7, 2024

View reviewed changes

misvivek force-pushed the 4552-empty-response-on-enable-decompression branch from 3589636 to 3603069 Compare November 7, 2024 19:55

misvivek requested a review from purnesh42H November 7, 2024 20:29

purnesh42H reviewed Nov 8, 2024

View reviewed changes

misvivek requested a review from purnesh42H November 19, 2024 02:55

Rebase master fork in my local

786724d

purnesh42H changed the title ~~grpc: fix getting empty response when compress enabled and maxReceiveMessageSize be maxInt64~~ grpc: fix receiving empty messages when compression is enabled and maxReceiveMessageSize is maxInt64 Nov 20, 2024

purnesh42H reviewed Nov 20, 2024

View reviewed changes

misvivek added 8 commits November 21, 2024 04:58

Merge branch 'grpc:master' into master

2bf22b3

rebase with master branch

eb85e40

rebase the branch

c830a6c

address the comment

32d1ca1

added bye compare

94f6a63

address size and compare

5bbf7d6

added size with wantMsg.Len

6dfb4a9

address the comment

fad8892

misvivek force-pushed the 4552-empty-response-on-enable-decompression branch from 47003c3 to fad8892 Compare November 21, 2024 05:36

purnesh42H reviewed Nov 21, 2024

View reviewed changes

misvivek added 2 commits November 21, 2024 10:16

address the comment

63934e6

changes done for file when want is nil

f3afb78

misvivek requested a review from purnesh42H November 28, 2024 08:30

misvivek added 2 commits December 4, 2024 06:51

chages as new rpc_util

8f51348

remove the+1 in mem.ReadAll

56f4538

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

added the num buf lice

b1bd3b3

dfawley closed this Dec 10, 2024

grpc: fix receiving empty messages when compression is enabled and maxReceiveMessageSize is maxInt64 #7753

grpc: fix receiving empty messages when compression is enabled and maxReceiveMessageSize is maxInt64 #7753

Conversation

misvivek commented Oct 17, 2024 • edited by purnesh42H Loading

linux-foundation-easycla bot commented Oct 17, 2024 • edited Loading

purnesh42H commented Oct 17, 2024

misvivek commented Oct 17, 2024

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

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

Choose a reason for hiding this comment

purnesh42H left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

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

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

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

misvivek Nov 8, 2024 • edited Loading

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

misvivek Nov 20, 2024 • edited Loading

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

misvivek commented Oct 17, 2024 •

edited by purnesh42H

Loading

linux-foundation-easycla bot commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

misvivek Nov 8, 2024 •

edited

Loading

misvivek Nov 20, 2024 •

edited

Loading