Skip to content

Commit

Permalink
Use gRPC StatusRuntimeException instead of StatusException. (#4658)
Browse files Browse the repository at this point in the history
Motivation:
We have changed to use `StatusException` for gRPC client in this PR: https://github.com/line/armeria/pull/4636/files#diff-39112577712e2be6800f84834abc33a690eed333cd96581b777e1e1cd1ccdadaR497

However, `StatusRuntimeException` is used more by the upstream gRPC and gRPC's stubs favor it.
```
/**
 * {@link Status} in Exception form, for propagating Status information via exceptions. This is
 * semantically equivalent to {@link StatusRuntimeException}, except for usage in APIs that promote
 * checked exceptions. gRPC's stubs favor {@code StatusRuntimeException}.
 */
public class StatusException extends Exception {...}
```
They are semantically equivalent and users handle the exceptions together.
grpc/grpc-java#4683
https://github.com/grpc/grpc-java/blob/master/api/src/main/java/io/grpc/Status.java#L404-L408

gRPC stub usually creates `StatusRuntimeException`:
https://github.com/grpc/grpc-java/blob/master/stub/src/main/java/io/grpc/stub/ClientCalls.java#L540-L544
https://github.com/grpc/grpc-java/blob/master/stub/src/main/java/io/grpc/stub/ServerCalls.java#L291

So we'd better to use `StatusRuntimeException` instead of `StatusException`. If so, we can also reduce creating the exception multiple times which is expensive due to `fillInStackTrace` call,

Modifications:
- Use `StatusRuntimeException` instead of `StatusException`.
- Add `StatusAndMetadata` that creates the `StatusRuntimeException` only once.

Result:
- `StatusRuntimeException` is used for aborting a request and logging.
  • Loading branch information
minwoox committed Feb 8, 2023
1 parent c11b7ab commit bfc1924
Show file tree
Hide file tree
Showing 13 changed files with 180 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public class HelloServiceImpl extends HelloServiceImplBase {
public void hello(HelloRequest request, StreamObserver<HelloReply> responseObserver) {
if (request.getName().isEmpty()) {
responseObserver.onError(
Status.FAILED_PRECONDITION.withDescription("Name cannot be empty").asException());
Status.FAILED_PRECONDITION.withDescription("Name cannot be empty").asRuntimeException());
} else {
responseObserver.onNext(buildReply(toMessage(request.getName())));
responseObserver.onCompleted();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import com.linecorp.armeria.internal.common.grpc.GrpcStatus;
import com.linecorp.armeria.internal.common.grpc.HttpStreamDeframer;
import com.linecorp.armeria.internal.common.grpc.MetadataUtil;
import com.linecorp.armeria.internal.common.grpc.StatusAndMetadata;
import com.linecorp.armeria.internal.common.grpc.TimeoutHeaderUtil;
import com.linecorp.armeria.internal.common.grpc.TransportStatusListener;
import com.linecorp.armeria.unsafe.grpc.GrpcUnsafeBufferUtil;
Expand Down Expand Up @@ -421,7 +422,7 @@ public void onNext(DeframedMessage message) {
}
} catch (Throwable t) {
final Status status = GrpcStatus.fromThrowable(t);
req.close(status.asException());
req.close(status.asRuntimeException());
close(status, new Metadata());
}

Expand Down Expand Up @@ -490,11 +491,12 @@ private void close(Status status, Metadata metadata) {
}

final RequestLogBuilder logBuilder = ctx.logBuilder();
logBuilder.responseContent(GrpcLogUtil.rpcResponse(status, firstResponse, metadata), null);
final StatusAndMetadata statusAndMetadata = new StatusAndMetadata(status, metadata);
logBuilder.responseContent(GrpcLogUtil.rpcResponse(statusAndMetadata, firstResponse), null);
if (status.isOk()) {
req.abort();
} else {
req.abort(status.asException(metadata));
req.abort(statusAndMetadata.asRuntimeException());
}
if (upstream != null) {
upstream.cancel();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.linecorp.armeria.common.RpcResponse;
import com.linecorp.armeria.common.annotation.Nullable;

import io.grpc.Metadata;
import io.grpc.MethodDescriptor;
import io.grpc.Status;

Expand Down Expand Up @@ -53,14 +52,14 @@ public static RpcRequest rpcRequest(MethodDescriptor<?, ?> method, String simple
}

/**
* Returns a {@link RpcResponse} corresponding to the given {@link Status} generated by the server.
* Returns a {@link RpcResponse} corresponding to the given {@link StatusAndMetadata}.
*/
public static RpcResponse rpcResponse(Status status, @Nullable Object message,
@Nullable Metadata metadata) {
public static RpcResponse rpcResponse(StatusAndMetadata statusAndMetadata, @Nullable Object message) {
final Status status = statusAndMetadata.status();
if (status.isOk()) {
return RpcResponse.of(message);
} else {
return RpcResponse.ofFailure(status.asException(metadata));
return RpcResponse.ofFailure(statusAndMetadata.asRuntimeException());
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.internal.common.grpc;

import com.linecorp.armeria.common.annotation.Nullable;

import io.grpc.Metadata;
import io.grpc.Status;
import io.grpc.StatusRuntimeException;

/**
* A class that is used to cache {@link StatusRuntimeException} created from
* {@link Status#asRuntimeException(Metadata)}.
*/
public final class StatusAndMetadata {

private final Status status;
@Nullable
private final Metadata metadata;
@Nullable
private StatusRuntimeException statusRuntimeException;

public StatusAndMetadata(Status status, @Nullable Metadata metadata) {
this.status = status;
this.metadata = metadata;
}

public Status status() {
return status;
}

@Nullable
public Metadata metadata() {
return metadata;
}

public StatusRuntimeException asRuntimeException() {
if (statusRuntimeException == null) {
statusRuntimeException = status.asRuntimeException(metadata);
}
return statusRuntimeException;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import com.linecorp.armeria.internal.common.grpc.GrpcMessageMarshaller;
import com.linecorp.armeria.internal.common.grpc.GrpcStatus;
import com.linecorp.armeria.internal.common.grpc.MetadataUtil;
import com.linecorp.armeria.internal.common.grpc.StatusAndMetadata;
import com.linecorp.armeria.internal.common.grpc.StatusExceptionConverter;
import com.linecorp.armeria.internal.common.grpc.protocol.GrpcTrailersUtil;
import com.linecorp.armeria.server.RequestTimeoutException;
Expand All @@ -81,7 +82,6 @@
import io.grpc.ServerCall;
import io.grpc.Status;
import io.grpc.Status.Code;
import io.grpc.StatusException;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufAllocator;
import io.netty.channel.EventLoop;
Expand Down Expand Up @@ -261,24 +261,30 @@ private void doClose(Status status, Metadata metadata, @Nullable Throwable excep

final void closeListener(Status newStatus, Metadata metadata, boolean completed,
boolean setResponseContent) {
closeListener(new StatusAndMetadata(newStatus, metadata), completed, setResponseContent);
}

final void closeListener(StatusAndMetadata statusAndMetadata, boolean completed,
boolean setResponseContent) {
if (!listenerClosed) {
listenerClosed = true;

if (!ctx.log().isAvailable(RequestLogProperty.REQUEST_CONTENT)) {
// Failed to deserialize a message into a request
ctx.logBuilder().requestContent(GrpcLogUtil.rpcRequest(method, simpleMethodName), null);
}

if (setResponseContent) {
ctx.logBuilder().responseContent(GrpcLogUtil.rpcResponse(newStatus, firstResponse(),
metadata), null);
ctx.logBuilder().responseContent(GrpcLogUtil.rpcResponse(statusAndMetadata, firstResponse()),
null);
}

if (!clientStreamClosed) {
clientStreamClosed = true;
if (newStatus.isOk()) {
if (statusAndMetadata.status().isOk()) {
req.abort();
} else {
req.abort(newStatus.asException(metadata));
req.abort(statusAndMetadata.asRuntimeException());
}
}

Expand All @@ -297,8 +303,7 @@ final void closeListener(Status newStatus, Metadata metadata, boolean completed,
}
// Transport error, not business logic error, so reset the stream.
if (!closeCalled) {
final StatusException statusException = newStatus.asException(metadata);
res.abort(statusException);
res.abort(statusAndMetadata.asRuntimeException());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.linecorp.armeria.common.stream.SubscriptionOption;
import com.linecorp.armeria.internal.common.grpc.GrpcLogUtil;
import com.linecorp.armeria.internal.common.grpc.HttpStreamDeframer;
import com.linecorp.armeria.internal.common.grpc.StatusAndMetadata;
import com.linecorp.armeria.internal.common.grpc.TransportStatusListener;
import com.linecorp.armeria.server.ServiceRequestContext;

Expand Down Expand Up @@ -210,14 +211,15 @@ void doClose(Status status, Metadata metadata, boolean completed) {
}
}

final StatusAndMetadata statusAndMetadata = new StatusAndMetadata(status, metadata);
// Set responseContent before closing stream to use responseCause in error handling
ctx.logBuilder().responseContent(GrpcLogUtil.rpcResponse(status, firstResponse, metadata), null);
ctx.logBuilder().responseContent(GrpcLogUtil.rpcResponse(statusAndMetadata, firstResponse), null);
try {
if (res.tryWrite(responseTrailers(ctx, status, metadata, trailersOnly))) {
res.close();
}
} finally {
closeListener(status, metadata, completed, false);
closeListener(statusAndMetadata, completed, false);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.linecorp.armeria.common.grpc.GrpcSerializationFormats;
import com.linecorp.armeria.common.grpc.GrpcStatusFunction;
import com.linecorp.armeria.internal.common.grpc.GrpcLogUtil;
import com.linecorp.armeria.internal.common.grpc.StatusAndMetadata;
import com.linecorp.armeria.server.ServiceRequestContext;

import io.grpc.CompressorRegistry;
Expand Down Expand Up @@ -139,6 +140,7 @@ public boolean isReady() {
@Override
void doClose(Status status, Metadata metadata, boolean completed) {
final ResponseHeaders responseHeaders = responseHeaders();
final StatusAndMetadata statusAndMetadata = new StatusAndMetadata(status, metadata);
final HttpResponse response;
try {
if (status.isOk()) {
Expand Down Expand Up @@ -167,12 +169,12 @@ void doClose(Status status, Metadata metadata, boolean completed) {
}

// Set responseContent before closing stream to use responseCause in error handling
ctx.logBuilder().responseContent(GrpcLogUtil.rpcResponse(status, responseMessage, metadata), null);
ctx.logBuilder().responseContent(GrpcLogUtil.rpcResponse(statusAndMetadata, responseMessage), null);
resFuture.complete(response);
} catch (Exception ex) {
resFuture.completeExceptionally(ex);
} finally {
closeListener(status, metadata, completed, false);
closeListener(statusAndMetadata, completed, false);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@
import io.grpc.ServerInterceptor;
import io.grpc.Status;
import io.grpc.Status.Code;
import io.grpc.StatusException;
import io.grpc.StatusRuntimeException;
import io.grpc.protobuf.ProtoUtils;
import io.grpc.stub.MetadataUtils;
Expand Down Expand Up @@ -148,7 +147,7 @@ class GrpcClientTest {
private static class UnitTestServiceImpl extends UnitTestServiceImplBase {
@Override
public void errorNoMessage(SimpleRequest request, StreamObserver<SimpleResponse> responseObserver) {
responseObserver.onError(Status.ABORTED.asException());
responseObserver.onError(Status.ABORTED.asRuntimeException());
}

@Override
Expand Down Expand Up @@ -665,8 +664,8 @@ void cancelAfterBegin() throws Exception {
assertThat(log.isComplete()).isTrue();
assertThat(log.responseContent()).isInstanceOf(RpcResponse.class);
final Throwable cause = ((RpcResponse) log.responseContent()).cause();
assertThat(cause).isInstanceOf(StatusException.class);
assertThat(((StatusException) cause).getStatus().getCode()).isEqualTo(Code.CANCELLED);
assertThat(cause).isInstanceOf(StatusRuntimeException.class);
assertThat(((StatusRuntimeException) cause).getStatus().getCode()).isEqualTo(Code.CANCELLED);
}

@Test
Expand Down Expand Up @@ -1343,11 +1342,12 @@ private static void assertResponseTimeoutExceptionOrDeadlineExceeded(@Nullable T
if (cause instanceof ResponseTimeoutException) {
return;
}
if (cause instanceof StatusException) {
assertThat(((StatusException) cause).getStatus().getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
if (cause instanceof StatusRuntimeException) {
assertThat(((StatusRuntimeException) cause).getStatus().getCode())
.isEqualTo(Code.DEADLINE_EXCEEDED);
return;
}
fail("cause must be a ResponseTimeoutException or a StatusException: ", cause);
fail("cause must be a ResponseTimeoutException or a StatusRuntimeException: ", cause);
}

@Test
Expand Down Expand Up @@ -1544,7 +1544,7 @@ void errorNoMessage() throws Exception {
assertThat(rpcReq.serviceName()).isEqualTo("armeria.grpc.testing.UnitTestService");
assertThat(rpcReq.method()).isEqualTo("ErrorNoMessage");
assertThat(rpcReq.params()).containsExactly(REQUEST_MESSAGE);
assertThat(cause).isInstanceOfSatisfying(StatusException.class, ex -> {
assertThat(cause).isInstanceOfSatisfying(StatusRuntimeException.class, ex -> {
assertThat(ex.getStatus()).isNotNull();
assertThat(ex.getStatus().getCode()).isEqualTo(Code.ABORTED);
assertThat(ex.getStatus().getDescription()).isNull();
Expand Down Expand Up @@ -1573,7 +1573,8 @@ void errorWithMessage() throws Exception {
assertThat(rpcReq.method()).isEqualTo("ErrorWithMessage");
assertThat(rpcReq.params()).containsExactly(REQUEST_MESSAGE);
assertThat(cause).isInstanceOfSatisfying(
StatusException.class, ex -> checkException.accept(ex.getStatus(), ex.getTrailers()));
StatusRuntimeException.class, ex -> checkException.accept(ex.getStatus(),
ex.getTrailers()));
});
}

Expand Down Expand Up @@ -1613,7 +1614,8 @@ void errorFromClientInStream() throws Exception {
assertThat(rpcReq.method()).isEqualTo("ErrorFromClient");
assertThat(rpcReq.params()).containsExactly(REQUEST_MESSAGE);
assertThat(cause).isInstanceOfSatisfying(
StatusException.class, ex -> checkException.accept(ex.getStatus(), ex.getTrailers()));
StatusRuntimeException.class, ex -> checkException.accept(ex.getStatus(),
ex.getTrailers()));
});
}

Expand Down Expand Up @@ -1749,7 +1751,7 @@ private void checkRequestLog(RequestLogChecker checker) throws Exception {

final Status grpcStatus;
if (rpcRes.cause() != null) {
grpcStatus = ((StatusException) rpcRes.cause()).getStatus();
grpcStatus = ((StatusRuntimeException) rpcRes.cause()).getStatus();
} else {
grpcStatus = null;
}
Expand Down

0 comments on commit bfc1924

Please sign in to comment.