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

Refactor GrpcStatus to implement GrpcExceptionHandlerFunction #5571

Merged
merged 32 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
c33f8ea
chore: refactor GrpcStatus to use it as DefaultGrpcExceptionHandlerFu…
jaeseung-bae Apr 5, 2024
30dd216
test: can use DefaultGrpcExceptionHandlerFunction for server builder …
jaeseung-bae Apr 5, 2024
866a041
chore: lint fix
jaeseung-bae Apr 5, 2024
3a5585f
chore: refactor, move exceptionHandling features from GrpcStatus to D…
jaeseung-bae Apr 12, 2024
1e0a4d2
Merge remote-tracking branch 'upstream/main' into refactor/grpc-status
jaeseung-bae Apr 12, 2024
0c804ef
chore: use DefaultGrpcExceptionHandlerFunction as fallback for both G…
jaeseung-bae Apr 14, 2024
a88f3c3
chore: try to add possibly missing description in case of replace_exc…
jaeseung-bae Apr 16, 2024
cccbe7d
Merge remote-tracking branch 'upstream/main' into refactor/grpc-status
jaeseung-bae Apr 16, 2024
5c56c80
chore: apply feedbacks
jaeseung-bae Apr 17, 2024
0d6f75c
chore: call peelAndUnwrap when exception thrown
jaeseung-bae Apr 17, 2024
d38f3be
Merge remote-tracking branch 'upstream/main' into HEAD
jaeseung-bae Apr 19, 2024
8604cf5
chore: UnwrappingGrpcExceptionHandlerFunction calls peelAndUnwrap at …
jaeseung-bae Apr 19, 2024
0bd68e0
chore: peelAndUnwrap is not supposed to receive null Throwable
jaeseung-bae Apr 20, 2024
63737b1
Update grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcExcept…
jaeseung-bae Apr 22, 2024
fb0692a
Update grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/U…
jaeseung-bae Apr 22, 2024
f68bd28
chore: apply feedbacks
jaeseung-bae Apr 22, 2024
894fbba
Merge branch 'main' into refactor/grpc-status
ikhoon Apr 25, 2024
7090d2c
Update grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExcept…
jaeseung-bae Apr 25, 2024
a35dc16
Update grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExcept…
jaeseung-bae Apr 25, 2024
a9a28ba
Update grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExcept…
jaeseung-bae Apr 25, 2024
0704f71
Update grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExcept…
jaeseung-bae Apr 25, 2024
e7b9908
chore: revert redundant wrapping code
jaeseung-bae May 3, 2024
7244059
Merge remote-tracking branch 'upstream/main' into refactor/grpc-status
jaeseung-bae May 18, 2024
10245db
chore: make enum for singleton instance
jaeseung-bae May 19, 2024
c717229
chore: rename ofDefault to of to meet recent convention
jaeseung-bae May 19, 2024
81f3d3a
chore: add comment
jaeseung-bae May 19, 2024
d1ff8b9
chore: add assertion for newStatus
jaeseung-bae May 19, 2024
9603904
chore: make it final, static
jaeseung-bae May 19, 2024
5b6dba4
chore: fix lint
jaeseung-bae May 19, 2024
99d5fbb
chore: refactoring for clean code
jaeseung-bae May 20, 2024
81c954d
Update grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClient…
jaeseung-bae May 20, 2024
0cef19b
Update grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/U…
jaeseung-bae May 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
import com.linecorp.armeria.common.grpc.protocol.ArmeriaMessageFramer;
import com.linecorp.armeria.internal.client.grpc.NullCallCredentials;
import com.linecorp.armeria.internal.client.grpc.NullGrpcClientStubFactory;
import com.linecorp.armeria.internal.common.grpc.GrpcStatus;
import com.linecorp.armeria.internal.common.grpc.DefaultGrpcExceptionHandlerFunction;
import com.linecorp.armeria.unsafe.grpc.GrpcUnsafeBufferUtil;

import io.grpc.CallCredentials;
Expand Down Expand Up @@ -173,8 +173,7 @@ public final class GrpcClientOptions {
* to a gRPC {@link Status}.
*/
public static final ClientOption<GrpcExceptionHandlerFunction> EXCEPTION_HANDLER =
ClientOption.define("EXCEPTION_HANDLER",
(ctx, cause, metadata) -> GrpcStatus.fromThrowable(cause));
ClientOption.define("EXCEPTION_HANDLER", DefaultGrpcExceptionHandlerFunction.INSTANCE);

private GrpcClientOptions() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@
import com.linecorp.armeria.common.util.TimeoutMode;
import com.linecorp.armeria.internal.client.DefaultClientRequestContext;
import com.linecorp.armeria.internal.client.grpc.protocol.InternalGrpcWebUtil;
import com.linecorp.armeria.internal.common.grpc.DefaultGrpcExceptionHandlerFunction;
import com.linecorp.armeria.internal.common.grpc.ForwardingCompressor;
import com.linecorp.armeria.internal.common.grpc.GrpcLogUtil;
import com.linecorp.armeria.internal.common.grpc.GrpcMessageMarshaller;
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;
Expand Down Expand Up @@ -248,9 +248,10 @@

final BiFunction<ClientRequestContext, Throwable, HttpResponse> errorResponseFactory =
(unused, cause) -> HttpResponse.ofFailure(
GrpcStatus.fromThrowable(exceptionHandler, ctx, cause, metadata)
.withDescription(cause.getMessage())
.asRuntimeException());
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, cause,
metadata)
.withDescription(cause.getMessage())
.asRuntimeException());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set DefaultGrpcExceptionHandlerFunction as a fallback exception handler for exceptionHandler when it is set in the GrpcClientBuilder like we do in ServerBuilder?

final ServerErrorHandler errorHandler;
if (this.errorHandler == null) {
errorHandler = ServerErrorHandler.ofDefault();
} else {
// Ensure that ServerErrorHandler never returns null by falling back to the default.
errorHandler = this.errorHandler.orElse(ServerErrorHandler.ofDefault());
}

The rest of code should not access to DefaultGrpcExceptionHandlerFunction and use the composed one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll modify to use DefaultGrpcExceptionHandlerFunction as fallback exception handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Global comments) If DefaultGrpcExceptionHandlerFunction is set as the fallback, should we remove DefaultGrpcExceptionHandlerFunction here and others?

Suggested change
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, cause,
metadata)
.withDescription(cause.getMessage())
.asRuntimeException());
exceptionHandler.apply(ctx, cause, metadata)
.withDescription(cause.getMessage())
.asRuntimeException());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied feedback. Thanks

final HttpResponse res = initContextAndExecuteWithFallback(
httpClient, ctx, endpointGroup, HttpResponse::of, errorResponseFactory);

Expand Down Expand Up @@ -424,7 +425,7 @@
new Metadata());
} else {
GrpcWebTrailers.set(ctx, trailers);
GrpcStatus.reportStatus(trailers, this);
DefaultGrpcExceptionHandlerFunction.reportStatus(trailers, this);
Copy link
Contributor

Choose a reason for hiding this comment

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

reportStatus is unrelated to GrpcExceptionHandlerFunction. Should we leave it in GrpcStatus and only move code handling exceptions to DefaultGrpcExceptionHandlerFunction?

Copy link
Contributor Author

@jaeseung-bae jaeseung-bae Apr 8, 2024

Choose a reason for hiding this comment

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

Thanks for the recommendation. I also think we should separate GrpcStatus and DefaultGrpcExceptionHandlerFunction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied feedback. Thanks

}
} finally {
buf.release();
Expand Down Expand Up @@ -454,7 +455,8 @@
});
} catch (Throwable t) {
final Metadata metadata = new Metadata();
close(GrpcStatus.fromThrowable(exceptionHandler, ctx, t, metadata), metadata);
close(DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, t, metadata),

Check warning on line 458 in grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java

View check run for this annotation

Codecov / codecov/patch

grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java#L458

Added line #L458 was not covered by tests
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
metadata);
}
}

Expand Down Expand Up @@ -511,7 +513,8 @@

private void closeWhenListenerThrows(Throwable t) {
final Metadata metadata = new Metadata();
closeWhenEos(GrpcStatus.fromThrowable(exceptionHandler, ctx, t, metadata), metadata);
closeWhenEos(DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, t, metadata),
metadata);
}

private void closeWhenEos(Status status, Metadata metadata) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,15 @@
/**
* Utilities for handling {@link Status} in Armeria.
*/
public final class GrpcStatus {

private static final Logger logger = LoggerFactory.getLogger(GrpcStatus.class);
public final class DefaultGrpcExceptionHandlerFunction implements GrpcExceptionHandlerFunction {
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
public static final GrpcExceptionHandlerFunction INSTANCE = new DefaultGrpcExceptionHandlerFunction();
private static final Logger logger = LoggerFactory.getLogger(DefaultGrpcExceptionHandlerFunction.class);

@Nullable
@Override
public Status apply(RequestContext ctx, Throwable cause, Metadata metadata) {
return fromThrowable(cause);
}

/**
* Converts the {@link Throwable} to a {@link Status}, taking into account exceptions specific to Armeria as
Expand Down Expand Up @@ -400,5 +406,5 @@ private static StackTraceElementProto serializeStackTraceElement(StackTraceEleme
return builder.build();
}

private GrpcStatus() {}
private DefaultGrpcExceptionHandlerFunction() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,16 @@
// Just mark trailers as received since a non-OK response may not have trailers.
trailersReceived = true;
transportStatusListener.transportReportStatus(
GrpcStatus.httpStatusToGrpcStatus(status.code()));
DefaultGrpcExceptionHandlerFunction.httpStatusToGrpcStatus(status.code()));
return;
}

final String grpcStatus = headers.get(GrpcHeaderNames.GRPC_STATUS);
if (grpcStatus != null) {
assert deframedStreamMessage != null;
trailersReceived = true;
GrpcStatus.reportStatusLater(headers, deframedStreamMessage, transportStatusListener);
DefaultGrpcExceptionHandlerFunction.reportStatusLater(headers, deframedStreamMessage,
transportStatusListener);
}

// Headers without grpc-status are the leading headers of a non-failing response, prepare to receive
Expand All @@ -122,7 +123,7 @@
} catch (Throwable t) {
final Metadata metadata = new Metadata();
transportStatusListener.transportReportStatus(
GrpcStatus.fromThrowable(exceptionHandler, ctx, t, metadata),
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, t, metadata),

Check warning on line 126 in grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframer.java

View check run for this annotation

Codecov / codecov/patch

grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframer.java#L126

Added line #L126 was not covered by tests
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
metadata);
return;
}
Expand All @@ -143,15 +144,17 @@
if (grpcStatus != null) {
assert deframedStreamMessage != null;
trailersReceived = true;
GrpcStatus.reportStatusLater(headers, deframedStreamMessage, transportStatusListener);
DefaultGrpcExceptionHandlerFunction.reportStatusLater(headers, deframedStreamMessage,
transportStatusListener);
}
}

@Override
public void processOnError(Throwable cause) {
final Metadata metadata = new Metadata();
transportStatusListener.transportReportStatus(
GrpcStatus.fromThrowable(exceptionHandler, ctx, cause, metadata), metadata);
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, cause, metadata),
ikhoon marked this conversation as resolved.
Show resolved Hide resolved
metadata);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@
import com.linecorp.armeria.common.stream.ClosedStreamException;
import com.linecorp.armeria.common.util.Exceptions;
import com.linecorp.armeria.common.util.SafeCloseable;
import com.linecorp.armeria.internal.common.grpc.DefaultGrpcExceptionHandlerFunction;
import com.linecorp.armeria.internal.common.grpc.ForwardingCompressor;
import com.linecorp.armeria.internal.common.grpc.ForwardingDecompressor;
import com.linecorp.armeria.internal.common.grpc.GrpcLogUtil;
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.StatusExceptionConverter;
import com.linecorp.armeria.internal.common.grpc.protocol.GrpcTrailersUtil;
Expand Down Expand Up @@ -213,16 +213,18 @@ public final void close(Throwable exception) {
public final void close(Throwable exception, boolean cancelled) {
exception = Exceptions.peel(exception);
final Metadata metadata = generateMetadataFromThrowable(exception);
final Status status = GrpcStatus.fromThrowable(exceptionHandler, ctx, exception, metadata);
final Status status = DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx,
exception, metadata);
close(new ServerStatusAndMetadata(status, metadata, false, cancelled), exception);
}

@Override
public final void close(Status status, Metadata metadata) {
final ServerStatusAndMetadata statusAndMetadata =
new ServerStatusAndMetadata(GrpcStatus.fromExceptionHandler(exceptionHandler, ctx,
status, metadata),
metadata, false);
new ServerStatusAndMetadata(
DefaultGrpcExceptionHandlerFunction.fromExceptionHandler(exceptionHandler, ctx,
status, metadata),
metadata, false);
close(statusAndMetadata);
}

Expand Down Expand Up @@ -565,7 +567,8 @@ public static HttpHeaders statusToTrailers(
trailersBuilder, status.getCode().value(), status.getDescription(), null);

if (ctx.config().verboseResponses() && status.getCause() != null) {
final ThrowableProto proto = GrpcStatus.serializeThrowable(status.getCause());
final ThrowableProto proto = DefaultGrpcExceptionHandlerFunction.serializeThrowable(
status.getCause());
trailersBuilder.add(GrpcHeaderNames.ARMERIA_GRPC_THROWABLEPROTO_BIN,
Base64.getEncoder().encodeToString(proto.toByteArray()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
import com.linecorp.armeria.common.logging.RequestLogProperty;
import com.linecorp.armeria.common.util.SafeCloseable;
import com.linecorp.armeria.common.util.TimeoutMode;
import com.linecorp.armeria.internal.common.grpc.GrpcStatus;
import com.linecorp.armeria.internal.common.grpc.DefaultGrpcExceptionHandlerFunction;
import com.linecorp.armeria.internal.common.grpc.MetadataUtil;
import com.linecorp.armeria.internal.common.grpc.TimeoutHeaderUtil;
import com.linecorp.armeria.internal.server.grpc.AbstractServerCall;
Expand Down Expand Up @@ -239,7 +239,8 @@
return HttpResponse.of(
(ResponseHeaders) AbstractServerCall.statusToTrailers(
ctx, defaultHeaders.get(serializationFormat).toBuilder(),
GrpcStatus.fromThrowable(exceptionHandler, ctx, e, metadata),
DefaultGrpcExceptionHandlerFunction.fromThrowable(exceptionHandler, ctx, e,

Check warning on line 242 in grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java

View check run for this annotation

Codecov / codecov/patch

grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java#L242

Added line #L242 was not covered by tests
metadata),
metadata));
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.internal.common.grpc.GrpcStatus;
import com.linecorp.armeria.internal.common.grpc.DefaultGrpcExceptionHandlerFunction;
import com.linecorp.armeria.server.ServiceRequestContext;

import io.grpc.Status;
Expand All @@ -37,7 +37,7 @@ public interface UnframedGrpcStatusMappingFunction {
* code.proto</a>.
*/
static UnframedGrpcStatusMappingFunction of() {
return (ctx, status, response) -> GrpcStatus.grpcStatusToHttpStatus(status);
return (ctx, status, response) -> DefaultGrpcExceptionHandlerFunction.grpcStatusToHttpStatus(status);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.linecorp.armeria.client.endpoint.EndpointGroup;
import com.linecorp.armeria.common.SerializationFormat;
import com.linecorp.armeria.common.grpc.GrpcSerializationFormats;
import com.linecorp.armeria.internal.common.grpc.DefaultGrpcExceptionHandlerFunction;

import io.grpc.CallOptions;
import io.grpc.Channel;
Expand Down Expand Up @@ -154,4 +155,14 @@ public <I, O> ClientCall<I, O> interceptCall(MethodDescriptor<I, O> method,
assertThat(clientParams.options().get(GrpcClientOptions.INTERCEPTORS))
.containsExactly(interceptorA, interceptorB);
}

@Test
void useDefaultGrpcExceptionHandlerFunction() {
final TestServiceBlockingStub client = GrpcClients.builder("http://foo.com").build(
TestServiceBlockingStub.class);

final ClientBuilderParams clientParams = Clients.unwrap(client, ClientBuilderParams.class);
assertThat(clientParams.options().get(GrpcClientOptions.EXCEPTION_HANDLER))
.isEqualTo(DefaultGrpcExceptionHandlerFunction.INSTANCE);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@
import com.linecorp.armeria.common.util.Exceptions;
import com.linecorp.armeria.common.util.ThreadFactories;
import com.linecorp.armeria.internal.common.RequestTargetCache;
import com.linecorp.armeria.internal.common.grpc.DefaultGrpcExceptionHandlerFunction;
import com.linecorp.armeria.internal.common.grpc.GrpcLogUtil;
import com.linecorp.armeria.internal.common.grpc.GrpcStatus;
import com.linecorp.armeria.internal.common.grpc.MetadataUtil;
import com.linecorp.armeria.internal.common.grpc.StreamRecorder;
import com.linecorp.armeria.internal.common.grpc.TestServiceImpl;
Expand Down Expand Up @@ -746,7 +746,8 @@ void cancelAfterBegin() throws Exception {
requestObserver.onError(new RuntimeException());
responseObserver.awaitCompletion();
assertThat(responseObserver.getValues()).isEmpty();
assertThat(GrpcStatus.fromThrowable(responseObserver.getError()).getCode()).isEqualTo(Code.CANCELLED);
assertThat(DefaultGrpcExceptionHandlerFunction.fromThrowable(responseObserver.getError())
.getCode()).isEqualTo(Code.CANCELLED);

final RequestLog log = requestLogQueue.take();
assertThat(log.isComplete()).isTrue();
Expand Down Expand Up @@ -780,7 +781,8 @@ void cancelAfterFirstResponse() throws Exception {
requestObserver.onError(new RuntimeException());
responseObserver.awaitCompletion(operationTimeoutMillis(), TimeUnit.MILLISECONDS);
assertThat(responseObserver.getValues()).hasSize(1);
assertThat(GrpcStatus.fromThrowable(responseObserver.getError()).getCode()).isEqualTo(Code.CANCELLED);
assertThat(DefaultGrpcExceptionHandlerFunction.fromThrowable(responseObserver.getError())
.getCode()).isEqualTo(Code.CANCELLED);

checkRequestLog((rpcReq, rpcRes, grpcStatus) -> {
assertThat(rpcReq.params()).containsExactly(request);
Expand Down Expand Up @@ -1413,7 +1415,7 @@ void deadlineExceededServerStreaming() throws Exception {
recorder.awaitCompletion();

assertThat(recorder.getError()).isNotNull();
assertThat(GrpcStatus.fromThrowable(recorder.getError()).getCode())
assertThat(DefaultGrpcExceptionHandlerFunction.fromThrowable(recorder.getError()).getCode())
.isEqualTo(Status.DEADLINE_EXCEEDED.getCode());

checkRequestLogError((headers, rpcReq, cause) -> {
Expand Down Expand Up @@ -1611,8 +1613,10 @@ void statusCodeAndMessage() throws Exception {
final ArgumentCaptor<Throwable> captor = ArgumentCaptor.forClass(Throwable.class);
verify(responseObserver,
timeout(operationTimeoutMillis())).onError(captor.capture());
assertThat(GrpcStatus.fromThrowable(captor.getValue()).getCode()).isEqualTo(Status.UNKNOWN.getCode());
assertThat(GrpcStatus.fromThrowable(captor.getValue()).getDescription()).isEqualTo(errorMessage);
assertThat(DefaultGrpcExceptionHandlerFunction.fromThrowable(captor.getValue()).getCode()).isEqualTo(
Status.UNKNOWN.getCode());
assertThat(DefaultGrpcExceptionHandlerFunction.fromThrowable(captor.getValue())
.getDescription()).isEqualTo(errorMessage);
verifyNoMoreInteractions(responseObserver);

checkRequestLog((rpcReq, rpcRes, grpcStatus) -> {
Expand Down
Loading
Loading