-
Notifications
You must be signed in to change notification settings - Fork 419
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
Improve error message when length exceeds receive limit #1247
Conversation
Motivation: The error message emitted when a received message exceeds the configured receive limit is non-obvious which make diagnosis more difficult than it needs to be. Modifications: - Add a new error rather than relying on a deserialization error - Use the 'resource exhausted' status code rather than 'internal error' when converting to a grpc status - Update tests. Result: More helpful error when received messages exceed permitted limit
d8eb46c
to
46c7d29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small notes... Looks good though.
@@ -92,6 +92,19 @@ public enum GRPCError { | |||
} | |||
} | |||
|
|||
/// The length of the received payload was longer than is permitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to be really nice to the user, we could point them to the config struct that they need to tune.
@@ -463,6 +463,8 @@ extension GRPCClientChannelHandler: ChannelInboundHandler { | |||
case let .decompressionLimitExceeded(compressedSize): | |||
return GRPCError.DecompressionLimitExceeded(compressedSize: compressedSize) | |||
.captureContext() | |||
case let .lengthExceedsLimit(underlyingError): | |||
return underlyingError.captureContext() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we create the error in LengthPrefixedMessageReader.swift
we already capture the context. Is it right, that we capture the context here again, even though we pass the original error onwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context just holds the error (and location info). Here we just have the wrapped error and no context. It's intentional that we capture it again, although the reason we have to do this is a bit gross: the error we're switching over is Equatable
but the error wrapper providing context is not Equatable
(because not all wrapped errors are Equatable
). The server doesn't wrap/unwrap the error like this so we still need to capture the context in LengthPrefixedMessageReader.swift
.
Motivation:
The error message emitted when a received message exceeds the configured
receive limit is non-obvious which make diagnosis more difficult than it
needs to be.
Modifications:
when converting to a grpc status
Result:
More helpful error when received messages exceed permitted limit