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

Incorrect grpc status code on exceeding max response limit #5818

Closed
yzfeng2020 opened this issue Jul 19, 2024 · 0 comments · Fixed by #5824
Closed

Incorrect grpc status code on exceeding max response limit #5818

yzfeng2020 opened this issue Jul 19, 2024 · 0 comments · Fixed by #5824
Labels
Milestone

Comments

@yzfeng2020
Copy link
Contributor

There seems to be an issue with the way gRPC status codes are handled when the max response limit is exceeded.The following line:

final HttpData responseBody = toPayload(responseMessage);

could throw AmeriaStatusRuntimeException when the max response limit is set. The response future is then completed with that exception directly, bypassing the exception handler. This is different from exceptions occurring within the handler, which are transformed into StatusRuntimeException in the close method:

public final void close(Throwable exception, boolean cancelled) {
exception = Exceptions.peel(exception);
final Metadata metadata = generateMetadataFromThrowable(exception);
final Status status = Status.fromThrowable(exception);
close(status, metadata, cancelled, exception);
}
.

As a result, the expected gRPC status code RESOURCE_EXHAUSTED is not being set when the response message exceeds the limit. Instead, I observe that the status code returned is INTERNAL_ERROR.

@ikhoon ikhoon added the defect label Jul 19, 2024
@ikhoon ikhoon added this to the 1.29.3 milestone Jul 19, 2024
@ikhoon ikhoon closed this as completed in f58ffb3 Jul 26, 2024
ikhoon added a commit that referenced this issue Jul 26, 2024
… is exceeded (#5824)

Motivation:

If a request or response message exceeds the maximum allowed message
length, `RESOURCE_EXHAUSTED` status should be returned to the client.

In `UnaryServerCall`, an exception raised `ArmeriaMessageFramer` is not
handled by `GrpcExceptionHandlerFunction`. As a result, the exception is
directly passed to the HTTP level `ServerErrorHandler` and a proper gRPC
status is not delivered to the client.

While I was fixing the bug, I found a lot of code that calls
`generateMetadataFromThrowable()` and then `fromThrowable()`. As the
same pattern was used repeatedly, I also refactored
`GrpcExceptionHandlerFunctionUtil` to return `StatusAndMetata` instead.

Modifications:

- Apply `GrpcExceptionHandlerFunction` when an exception is raised in
`UnaryServerCall.doClose()`
- Refactor `GrpcExceptionHandlerFunctionUtil` and its calling site.
- Add `InternalGrpcExceptionHandler` to wrap
`GrpcExceptionHandlerFunction`.
- Remove the incorrect `@Nullable` annotation for
`GrpcExceptionHandlerFunction`.
- Remove `ServerStatusAndMetadata.setResponseContent()` and use
`RESPONSE_CONTENT` property for simpliciy.

Result:

- `GrpcService` now correctly returns `RESOURCE_EXHAUSTED` when a
response message exceeds the maximum allowed length.
- Closes #5818
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants