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

Fix outbound message size checking #10739

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Dec 9, 2023

This fixes two bugs in outbound message size checking:

  • When the check failed, the thrown StatusRuntimeException with a status code of RESOURCE_EXHAUSTED was being caught and rewrapped in another StatusRuntimeException but this time with a status code of UNKNOWN, and the error message was lost.
  • This applies the max outbound message size check to messages prior to, and after compression, since compression of a message that is smaller than the maximum send size can result in a larger message that exceeds the maximum send size. See Should maxOutboundMessageSize be applied to compressed message bytes, as well as uncompressed message bytes? #10738.

Closes #10738.

This fixes two bugs in outbound message size checking:

* When thet checke failed, the thrown StatusRuntimeException with a status code
  of RESOURCE_EXHAUSTED was been caught and rewrapped in another
  StatusRuntimeException but this time with status code UNKNOWN.
* This applies the max outbound message size check to messages prior to, and
  after compression, since compression of a message that is smaller than the
  maximum send size can result in a larger message that exceeds the maximum
  send size.
@ejona86
Copy link
Member

ejona86 commented Dec 13, 2023

rewrapped in another StatusRuntimeException but this time with a status code of UNKNOWN

It should have been INTERNAL and the original exception in getCause()/causal chain. But yeah, better not to re-wrap it.

We really don't care that much about this level of precision of the outbound maximum message size, but that just means we wouldn't spend much effort on it. This is a very clean change, so easy to accept.

@ejona86
Copy link
Member

ejona86 commented Dec 13, 2023

This fixes #10738

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 13, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 13, 2023
Copy link
Contributor

@YifeiZhuang YifeiZhuang left a comment

Choose a reason for hiding this comment

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

LGTM

@YifeiZhuang YifeiZhuang merged commit 67b67f8 into grpc:master Dec 18, 2023
13 of 14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should maxOutboundMessageSize be applied to compressed message bytes, as well as uncompressed message bytes?
4 participants