rpc_util: Change error message to indicate size after decompression #4918
Conversation
rpc_util.go
Outdated
| @@ -570,7 +570,7 @@ func (p *parser) recvMsg(maxReceiveMessageSize int) (pf payloadFormat, msg []byt | |||
| return 0, nil, status.Errorf(codes.ResourceExhausted, "grpc: received message larger than max length allowed on current machine (%d vs. %d)", length, maxInt) | |||
| } | |||
| if int(length) > maxReceiveMessageSize { | |||
| return 0, nil, status.Errorf(codes.ResourceExhausted, "grpc: received message larger than max (%d vs. %d)", length, maxReceiveMessageSize) | |||
| return 0, nil, status.Errorf(codes.ResourceExhausted, "grpc: received message after decompression larger than max (%d vs. %d)", length, maxReceiveMessageSize) | |||
There was a problem hiding this comment.
This isn't correct. This is the raw message before decompression. recvAndDecompress calls this and decompresses this payload as a separate step.
There was a problem hiding this comment.
Also note that we can't call this the "compressed" message, since we don't know whether it's compressed in the first place. pf can tell us whether it's compressed if we can't come up with a generic enough message to convey the idea properly.
There was a problem hiding this comment.
@dfawley
I have a small question, I was writing this fix after reading this comment here:
In this, the issue creator says that he is "compressing" an 8 MB message to 4 MB and then sending it over.
Now, in our logic, since we are decompressing it, later on, is it possible that your current logic and error used are absolutely right and the message was not compressed in the first place?
If so, then this PR is irrelevant.
There was a problem hiding this comment.
We check the message size twice. Here (not yet decompressed) and here after decompressing:
Line 721 in 3b94303
And if a message isn't compressed, it seems like we might be checking the size twice, which we could clean up as part of this. The issue is about clarifying whether it's the compressed message or the decompressed message size was over the limit (if the message is compressed).
|
@dfawley Thank you for the review and pointers. I have moved up this check so that we recheck the size once decompression is completed, also I have removed the message change from the first check itself. Ps: also, could you help me in adding labels to this PR? Mergeable bot seems to require it. |
|
Just noticed the vet error: |
|
@dfawley Removed that, thanks for pointing out |
Fixes #4761
RELEASE NOTES: