From 00a662e304556b884e4a780022157d274b62780c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 28 Jun 2024 10:07:03 +0000 Subject: [PATCH 1/6] Update public suffix list (#5791) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- core/src/main/resources/com/linecorp/armeria/public_suffixes.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt index 125aeefb178..07ac4e679be 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -3874,7 +3874,6 @@ ink ino.kochi.jp instance.datadetect.com instances.spawn.cc -instantcloud.cn institute insurance insurance.aero From d30dff0db75915e160b31a427f6f3c0c25fc003b Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 29 Jun 2024 10:06:25 +0000 Subject: [PATCH 2/6] Update public suffix list (#5792) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- .../main/resources/com/linecorp/armeria/public_suffixes.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt index 07ac4e679be..c4c7c8eef5b 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -41,7 +41,6 @@ *.compute.amazonaws.com *.compute.amazonaws.com.cn *.compute.estate -*.cprapid.com *.cryptonomic.net *.customer-oci.com *.d.crm.dev @@ -1822,6 +1821,7 @@ courses coz.br cpa cpa.pro +cprapid.com cpserver.com cq.cn cr @@ -2102,6 +2102,7 @@ duckdns.org dunlop dupont durban +durumis.com dvag dvr dvrcam.info From 814d849139af030fdbd53e13fe960ad3761e3d33 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 2 Jul 2024 10:07:14 +0000 Subject: [PATCH 3/6] Update public suffix list (#5795) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- .../resources/com/linecorp/armeria/public_suffixes.txt | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt index c4c7c8eef5b..705fc8081c0 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -1375,6 +1375,7 @@ cf cf-ipfs.com cfa cfd +cfolks.pl cg ch ch.eu.org @@ -1827,6 +1828,7 @@ cq.cn cr cr.it cr.ua +craft.me crafting.xyz cranky.jp crap.jp @@ -2945,7 +2947,6 @@ fylkesbibl.no fyresdal.no g.bg g.se -g.vbrplsbx.io g12.br ga ga.us @@ -3438,6 +3439,12 @@ hasuda.saitama.jp hasura-app.io hasura.app hasvik.no +hateblo.jp +hatenablog.com +hatenablog.jp +hatenadiary.com +hatenadiary.jp +hatenadiary.org hatinh.vn hatogaya.saitama.jp hatoyama.saitama.jp From 191fb7db981d1d47de8ce50be0b0f6d5d9dfa8ee Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Thu, 4 Jul 2024 18:44:03 +0900 Subject: [PATCH 4/6] Fix `*ServiceBindingBuilder` to return the correct self type (#5797) Motivation: While refactoring Central Dogma, I found that some methods of service binding builders return internal classes or super types. Modifications: - Pass `SELF` type to super classes to return the class itself Result: Service binding builder methods now correctly return the self-types. --- ...textPathAnnotatedServiceConfigSetters.java | 5 +- ...tractContextPathServiceBindingBuilder.java | 2 +- .../server/AbstractServiceBindingBuilder.java | 13 +-- .../test/ServiceBuilderSelfTypeTest.java | 92 +++++++++++++++++++ 4 files changed, 96 insertions(+), 16 deletions(-) create mode 100644 core/src/test/java/com/linecorp/armeria/server/contextpath/test/ServiceBuilderSelfTypeTest.java diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathAnnotatedServiceConfigSetters.java b/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathAnnotatedServiceConfigSetters.java index 55be27575ea..2fbcb36865d 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathAnnotatedServiceConfigSetters.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathAnnotatedServiceConfigSetters.java @@ -23,8 +23,7 @@ abstract class AbstractContextPathAnnotatedServiceConfigSetters , T extends AbstractContextPathServicesBuilder> - extends AbstractAnnotatedServiceConfigSetters< - AbstractContextPathAnnotatedServiceConfigSetters> { + extends AbstractAnnotatedServiceConfigSetters { private final T builder; private final Set contextPaths; @@ -42,7 +41,7 @@ abstract class AbstractContextPathAnnotatedServiceConfigSetters * If path prefix is not set then this service is registered to handle requests matching * {@code /} */ - T build(Object service) { + public T build(Object service) { requireNonNull(service, "service"); service(service); contextPaths(contextPaths); diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServiceBindingBuilder.java index 99a022bffbb..5d457bede56 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServiceBindingBuilder.java @@ -21,7 +21,7 @@ abstract class AbstractContextPathServiceBindingBuilder , T extends AbstractContextPathServicesBuilder> - extends AbstractServiceBindingBuilder> { + extends AbstractServiceBindingBuilder { private final T contextPathServicesBuilder; diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java b/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java index dbb3f38eafc..34edc943299 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractServiceBindingBuilder.java @@ -44,8 +44,7 @@ * @see VirtualHostServiceBindingBuilder */ abstract class AbstractServiceBindingBuilder> - extends AbstractBindingBuilder - implements ServiceConfigSetters> { + extends AbstractBindingBuilder implements ServiceConfigSetters { private final DefaultServiceConfigSetters defaultServiceConfigSetters = new DefaultServiceConfigSetters(); @@ -261,14 +260,4 @@ final void build0(HttpService service) { } } } - - final void build0(HttpService service, Route mappedRoute) { - final List routes = buildRouteList(ImmutableSet.of()); - assert routes.size() == 1; // Only one route is set via addRoute(). - final HttpService decoratedService = defaultServiceConfigSetters.decorator().apply(service); - final ServiceConfigBuilder serviceConfigBuilder = - defaultServiceConfigSetters.toServiceConfigBuilder(routes.get(0), "/", decoratedService); - serviceConfigBuilder.addMappedRoute(mappedRoute); - serviceConfigBuilder(serviceConfigBuilder); - } } diff --git a/core/src/test/java/com/linecorp/armeria/server/contextpath/test/ServiceBuilderSelfTypeTest.java b/core/src/test/java/com/linecorp/armeria/server/contextpath/test/ServiceBuilderSelfTypeTest.java new file mode 100644 index 00000000000..988790efad9 --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/server/contextpath/test/ServiceBuilderSelfTypeTest.java @@ -0,0 +1,92 @@ +/* + * Copyright 2024 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.server.contextpath.test; + +import org.junit.jupiter.api.Test; + +import com.linecorp.armeria.server.ContextPathAnnotatedServiceConfigSetters; +import com.linecorp.armeria.server.ContextPathServiceBindingBuilder; +import com.linecorp.armeria.server.ContextPathServicesBuilder; +import com.linecorp.armeria.server.Server; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.server.ServiceBindingBuilder; +import com.linecorp.armeria.server.VirtualHostBuilder; +import com.linecorp.armeria.server.VirtualHostContextPathAnnotatedServiceConfigSetters; +import com.linecorp.armeria.server.VirtualHostContextPathServiceBindingBuilder; +import com.linecorp.armeria.server.VirtualHostContextPathServicesBuilder; + +class ServiceBuilderSelfTypeTest { + + // A non-existent package is used to check if the API is exposed publicly. + + @Test + void contextPathAnnotatedServiceConfigSetters() { + final ContextPathAnnotatedServiceConfigSetters setters = + Server.builder() + .contextPath("/foo") + .annotatedService() + .addHeader("X-foo", "bar"); + final ContextPathServicesBuilder contextPathServicesBuilder = setters.build(new Object()); + final ServerBuilder serverBuilder = contextPathServicesBuilder.and(); + } + + @Test + void virtualHostContextPathAnnotatedServiceConfigSetters() { + final VirtualHostContextPathAnnotatedServiceConfigSetters setters = + Server.builder() + .virtualHost("foo.com") + .contextPath("/foo") + .annotatedService() + .addHeader("X-foo", "bar"); + final VirtualHostContextPathServicesBuilder contextPathServicesBuilder = setters.build(new Object()); + final VirtualHostBuilder serverBuilder = contextPathServicesBuilder.and(); + } + + @Test + void serviceBindingBuilder() { + final ServiceBindingBuilder serviceBindingBuilder = + Server.builder() + .route() + .path("/"); + final ServiceBindingBuilder serviceBindingBuilder1 = serviceBindingBuilder.decorator( + (delegate, ctx, req) -> null); + serviceBindingBuilder1.build((ctx, req) -> null); + } + + @Test + void contextPathServiceBindingBuilder() { + final ContextPathServiceBindingBuilder builder = + Server.builder() + .contextPath("/foo") + .route() + .path("/") + .addHeader("X-foo", "bar"); + builder.build((ctx, req) -> null); + } + + @Test + void virtualHostContextPathServiceBindingBuilder() { + final VirtualHostContextPathServiceBindingBuilder builder = + Server.builder() + .virtualHost("foo.com") + .contextPath("/foo") + .route() + .path("/") + .addHeader("X-foo", "bar"); + builder.build((ctx, req) -> null); + } +} From 608d61c1fbb5c2c235b52eee8a9c6448bffa395b Mon Sep 17 00:00:00 2001 From: jrhee17 Date: Fri, 5 Jul 2024 12:58:46 +0900 Subject: [PATCH 5/6] `WeightRampingUpStrategy` uses locks to avoid occasional selection failure for `XdsEndpointGroup` (#5799) Motivation: https://github.com/line/armeria/issues/5749 While re-investigating this issue, I found that the probably cause is the changes introduced by https://github.com/line/armeria/pull/5693 Previously, when `updateEndpoints` is called the `endpointSelector` was initialized from the same thread https://github.com/line/armeria/blob/da7f76a7eebf9b03a51ff68a9245269eee4a5a6d/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategy.java#L147-L150 However after the PR above, `updateEndpoints` doesn't update the `endpointSelector` immediately and instead schedules an update task https://github.com/line/armeria/blob/191fb7db981d1d47de8ce50be0b0f6d5d9dfa8ee/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategy.java#L150 For this reason, even if `updateEndpoints` is called the `endpointSelector` will remain as the `EMPTY_SELECTOR`. https://github.com/line/armeria/blob/191fb7db981d1d47de8ce50be0b0f6d5d9dfa8ee/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategy.java#L128 Normally, this isn't an issue since `refreshEndpoints` will be called after the call to `updateEndpoints` which will in turn complete the `select` call. However, `XdsEndpointGroup` is composed of multiple internal `EndpointGroup`s and only `selectNow` is called. Because the initialization event is never propagated to the `XdsEndpointSelector` occasional failures can occur. https://github.com/line/armeria/blob/191fb7db981d1d47de8ce50be0b0f6d5d9dfa8ee/xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/DefaultLoadBalancer.java#L61-L84 Modifications: - Use locks instead of event loops for `updateEndpoints` Result: - Endpoint selection calls to `WeightRampingUpStrategy` succeeds before the first call to `updateEndpoints` for `XdsEndpointGroup`. --- .../endpoint/AbstractEndpointSelector.java | 20 ++++------ .../endpoint/WeightRampingUpStrategy.java | 40 +++++++++++++------ .../endpoint/WeightedRoundRobinStrategy.java | 5 +-- .../common/util/ReentrantShortLock.java | 6 +++ 4 files changed, 43 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java index 7503a2b3ae0..3761e42dc32 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java @@ -147,24 +147,20 @@ protected final void initialize() { private void refreshEndpoints(List endpoints) { // Allow subclasses to update the endpoints first. - updateNewEndpoints(endpoints).handle((ignored, e) -> { - lock.lock(); - try { - pendingFutures.removeIf(ListeningFuture::tryComplete); - } finally { - lock.unlock(); - } - return null; - }); + updateNewEndpoints(endpoints); + lock.lock(); + try { + pendingFutures.removeIf(ListeningFuture::tryComplete); + } finally { + lock.unlock(); + } } /** * Invoked when the {@link EndpointGroup} has been updated. */ @UnstableApi - protected CompletableFuture updateNewEndpoints(List endpoints) { - return UnmodifiableFuture.completedFuture(null); - } + protected void updateNewEndpoints(List endpoints) {} private void addPendingFuture(ListeningFuture future) { lock.lock(); diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategy.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategy.java index 70f22704a3b..9721ce10a60 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategy.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategy.java @@ -33,7 +33,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.function.Supplier; @@ -50,6 +49,7 @@ import com.linecorp.armeria.common.CommonPools; import com.linecorp.armeria.common.util.ListenableAsyncCloseable; import com.linecorp.armeria.common.util.Ticker; +import com.linecorp.armeria.internal.common.util.ReentrantShortLock; import io.netty.util.concurrent.EventExecutor; import it.unimi.dsi.fastutil.objects.Object2LongOpenHashMap; @@ -134,6 +134,7 @@ final class RampingUpEndpointWeightSelector extends AbstractEndpointSelector { @VisibleForTesting final Map rampingUpWindowsMap = new HashMap<>(); private Object2LongOpenHashMap endpointCreatedTimestamps = new Object2LongOpenHashMap<>(); + private final ReentrantShortLock lock = new ReentrantShortLock(true); RampingUpEndpointWeightSelector(EndpointGroup endpointGroup, EventExecutor executor) { super(endpointGroup); @@ -145,9 +146,14 @@ final class RampingUpEndpointWeightSelector extends AbstractEndpointSelector { } @Override - protected CompletableFuture updateNewEndpoints(List endpoints) { - // Use the executor so the order of endpoints change is guaranteed. - return CompletableFuture.runAsync(() -> updateEndpoints(endpoints), executor); + protected void updateNewEndpoints(List endpoints) { + // Use a lock so the order of endpoints change is guaranteed. + lock.lock(); + try { + updateEndpoints(endpoints); + } finally { + lock.unlock(); + } } private long computeCreateTimestamp(Endpoint endpoint) { @@ -245,14 +251,19 @@ private long initialDelayNanos(long windowIndex) { } private void updateWeightAndStep(long window) { - final EndpointsRampingUpEntry entry = rampingUpWindowsMap.get(window); - assert entry != null; - final Set endpointAndSteps = entry.endpointAndSteps(); - updateWeightAndStep(endpointAndSteps); - if (endpointAndSteps.isEmpty()) { - rampingUpWindowsMap.remove(window).scheduledFuture.cancel(true); + lock.lock(); + try { + final EndpointsRampingUpEntry entry = rampingUpWindowsMap.get(window); + assert entry != null; + final Set endpointAndSteps = entry.endpointAndSteps(); + updateWeightAndStep(endpointAndSteps); + if (endpointAndSteps.isEmpty()) { + rampingUpWindowsMap.remove(window).scheduledFuture.cancel(true); + } + buildEndpointSelector(); + } finally { + lock.unlock(); } - buildEndpointSelector(); } private void updateWeightAndStep(Set endpointAndSteps) { @@ -268,7 +279,12 @@ private void updateWeightAndStep(Set endpointAndSteps) { } private void close() { - rampingUpWindowsMap.values().forEach(e -> e.scheduledFuture.cancel(true)); + lock.lock(); + try { + rampingUpWindowsMap.values().forEach(e -> e.scheduledFuture.cancel(true)); + } finally { + lock.unlock(); + } } } diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightedRoundRobinStrategy.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightedRoundRobinStrategy.java index 61b1181222b..2a92ba93258 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightedRoundRobinStrategy.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightedRoundRobinStrategy.java @@ -20,7 +20,6 @@ import java.util.Comparator; import java.util.List; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicInteger; import com.google.common.collect.ImmutableList; @@ -29,7 +28,6 @@ import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.client.Endpoint; import com.linecorp.armeria.common.annotation.Nullable; -import com.linecorp.armeria.common.util.UnmodifiableFuture; final class WeightedRoundRobinStrategy implements EndpointSelectionStrategy { @@ -64,12 +62,11 @@ private static final class WeightedRoundRobinSelector extends AbstractEndpointSe } @Override - protected CompletableFuture updateNewEndpoints(List endpoints) { + protected void updateNewEndpoints(List endpoints) { final EndpointsAndWeights endpointsAndWeights = this.endpointsAndWeights; if (endpointsAndWeights == null || endpointsAndWeights.endpoints != endpoints) { this.endpointsAndWeights = new EndpointsAndWeights(endpoints); } - return UnmodifiableFuture.completedFuture(null); } @Override diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java b/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java index 626ea48245a..fee115a5b07 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/util/ReentrantShortLock.java @@ -28,6 +28,12 @@ public class ReentrantShortLock extends ReentrantLock { private static final long serialVersionUID = 8999619612996643502L; + public ReentrantShortLock() {} + + public ReentrantShortLock(boolean fair) { + super(fair); + } + @Override public void lock() { super.lock(); From 72ebfe12abf7804723a490fe7acfe3a45d46089f Mon Sep 17 00:00:00 2001 From: minux Date: Fri, 5 Jul 2024 13:00:28 +0900 Subject: [PATCH 6/6] Fix handling of exceptions in `GrpcExceptionHandlerFunction`. (#5796) Motivation: This PR addresses two issues with the `GrpcExceptionHandlerFunction`: - Peel the exception when calling `apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata)`. - Convert the cause to a `Status` via `Status.fromThrowable(cause)` before calling the `apply()` method. The reasoning behind the second change is to resolve the behavior discrepancy between a `StatusRuntimeException` being thrown and a `StatusRuntimeException` being handled with `onError`. When the exception is handled with `onError`, it is converted to a `Status` by the gRPC upstream, and `status.getCause()` is specified when calling the `apply()` method. However, when a `StatusRuntimeException` is thrown, the `StatusRuntimeException` is passed as is. Modifications: - Fixed to pass the peeled exception in `UnwrappingGrpcExceptionHandlerFunction`. - Converted the cause to a `Status` using `Status.fromThrowable(cause)` before calling the `apply()` method. Result: - The peeled exception is passed for `GrpcExceptionHandlerFunction.apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata)` method. --- .../blog/grpc/GrpcExceptionHandler.java | 2 +- .../kotlin/CoroutineServerInterceptorTest.kt | 2 +- .../client/grpc/GrpcClientBuilder.java | 4 +- .../client/grpc/GrpcClientOptions.java | 4 +- .../DefaultGrpcExceptionHandlerFunction.java | 5 +- .../GoogleGrpcExceptionHandlerFunction.java | 2 +- .../grpc/GrpcExceptionHandlerFunction.java | 8 +- .../client/grpc/ArmeriaClientCall.java | 21 +++-- ... => GrpcExceptionHandlerFunctionUtil.java} | 37 +++++--- .../common/grpc/HttpStreamDeframer.java | 12 ++- .../server/grpc/AbstractServerCall.java | 30 +++--- .../server/grpc/ServerStatusAndMetadata.java | 4 +- .../server/grpc/FramedGrpcService.java | 21 +++-- .../server/grpc/GrpcServiceBuilder.java | 4 +- .../armeria/server/grpc/HandlerRegistry.java | 4 +- .../armeria/client/grpc/GrpcClientTest.java | 10 +- ...faultGrpcExceptionHandlerFunctionTest.java | 5 +- ...pcExceptionHandlerFunctionBuilderTest.java | 18 ++-- .../common/grpc/HttpStreamDeframerTest.java | 3 +- .../internal/common/grpc/TestServiceImpl.java | 3 +- ...rpcExceptionHandlerAnnotationOnlyTest.java | 4 +- .../GrpcExceptionHandlerFunctionUtilTest.java | 94 +++++++++++++++++++ .../server/grpc/GrpcExceptionHandlerTest.java | 8 +- 23 files changed, 210 insertions(+), 95 deletions(-) rename grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/{UnwrappingGrpcExceptionHandleFunction.java => GrpcExceptionHandlerFunctionUtil.java} (51%) create mode 100644 grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerFunctionUtilTest.java diff --git a/examples/tutorials/grpc/src/main/java/example/armeria/server/blog/grpc/GrpcExceptionHandler.java b/examples/tutorials/grpc/src/main/java/example/armeria/server/blog/grpc/GrpcExceptionHandler.java index db748bc8ef6..91feda5fd1e 100644 --- a/examples/tutorials/grpc/src/main/java/example/armeria/server/blog/grpc/GrpcExceptionHandler.java +++ b/examples/tutorials/grpc/src/main/java/example/armeria/server/blog/grpc/GrpcExceptionHandler.java @@ -11,7 +11,7 @@ class GrpcExceptionHandler implements GrpcExceptionHandlerFunction { @Nullable @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { if (cause instanceof IllegalArgumentException) { return Status.INVALID_ARGUMENT.withCause(cause); } diff --git a/grpc-kotlin/src/test/kotlin/com/linecorp/armeria/server/grpc/kotlin/CoroutineServerInterceptorTest.kt b/grpc-kotlin/src/test/kotlin/com/linecorp/armeria/server/grpc/kotlin/CoroutineServerInterceptorTest.kt index 935c53889db..6573dc1f63b 100644 --- a/grpc-kotlin/src/test/kotlin/com/linecorp/armeria/server/grpc/kotlin/CoroutineServerInterceptorTest.kt +++ b/grpc-kotlin/src/test/kotlin/com/linecorp/armeria/server/grpc/kotlin/CoroutineServerInterceptorTest.kt @@ -230,7 +230,7 @@ internal class CoroutineServerInterceptorTest { val exceptionHandler = GrpcExceptionHandlerFunction { _: RequestContext, - _: Status?, + _: Status, throwable: Throwable, _: Metadata, -> diff --git a/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java b/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java index 039761f0511..7b9c59e5baa 100644 --- a/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java +++ b/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientBuilder.java @@ -75,7 +75,6 @@ import com.linecorp.armeria.common.grpc.GrpcSerializationFormats; import com.linecorp.armeria.common.grpc.protocol.ArmeriaMessageDeframer; import com.linecorp.armeria.common.grpc.protocol.ArmeriaMessageFramer; -import com.linecorp.armeria.internal.common.grpc.UnwrappingGrpcExceptionHandleFunction; import com.linecorp.armeria.unsafe.grpc.GrpcUnsafeBufferUtil; import io.grpc.CallCredentials; @@ -419,8 +418,7 @@ public T build(Class clientType) { option(INTERCEPTORS.newValue(clientInterceptors)); } if (exceptionHandler != null) { - option(EXCEPTION_HANDLER.newValue(new UnwrappingGrpcExceptionHandleFunction(exceptionHandler.orElse( - GrpcExceptionHandlerFunction.of())))); + option(EXCEPTION_HANDLER.newValue(exceptionHandler.orElse(GrpcExceptionHandlerFunction.of()))); } final Object client; diff --git a/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java b/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java index 09df88b849c..278b19844a5 100644 --- a/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java +++ b/grpc/src/main/java/com/linecorp/armeria/client/grpc/GrpcClientOptions.java @@ -37,7 +37,6 @@ 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.UnwrappingGrpcExceptionHandleFunction; import com.linecorp.armeria.unsafe.grpc.GrpcUnsafeBufferUtil; import io.grpc.CallCredentials; @@ -174,8 +173,7 @@ public final class GrpcClientOptions { * to a gRPC {@link Status}. */ public static final ClientOption EXCEPTION_HANDLER = - ClientOption.define("EXCEPTION_HANDLER", new UnwrappingGrpcExceptionHandleFunction( - GrpcExceptionHandlerFunction.of())); + ClientOption.define("EXCEPTION_HANDLER", GrpcExceptionHandlerFunction.of()); /** * Sets whether to respect the marshaller specified in gRPC {@link MethodDescriptor}. diff --git a/grpc/src/main/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunction.java b/grpc/src/main/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunction.java index eb17a000524..4a0203b19e4 100644 --- a/grpc/src/main/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunction.java +++ b/grpc/src/main/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunction.java @@ -27,7 +27,6 @@ import com.linecorp.armeria.common.ContentTooLargeException; import com.linecorp.armeria.common.RequestContext; import com.linecorp.armeria.common.TimeoutException; -import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.stream.ClosedStreamException; import com.linecorp.armeria.server.RequestTimeoutException; import com.linecorp.armeria.server.ServiceRequestContext; @@ -46,8 +45,8 @@ enum DefaultGrpcExceptionHandlerFunction implements GrpcExceptionHandlerFunction * well and the protocol package. */ @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { - if (status != null && status.getCode() != Code.UNKNOWN) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { + if (status.getCode() != Code.UNKNOWN) { return status; } final Status s = Status.fromThrowable(cause); diff --git a/grpc/src/main/java/com/linecorp/armeria/common/grpc/GoogleGrpcExceptionHandlerFunction.java b/grpc/src/main/java/com/linecorp/armeria/common/grpc/GoogleGrpcExceptionHandlerFunction.java index 4061b417c3f..9d32de1003e 100644 --- a/grpc/src/main/java/com/linecorp/armeria/common/grpc/GoogleGrpcExceptionHandlerFunction.java +++ b/grpc/src/main/java/com/linecorp/armeria/common/grpc/GoogleGrpcExceptionHandlerFunction.java @@ -40,7 +40,7 @@ public interface GoogleGrpcExceptionHandlerFunction extends GrpcExceptionHandler @Nullable @Override - default Status apply(RequestContext ctx, @Nullable Status status, Throwable throwable, Metadata metadata) { + default Status apply(RequestContext ctx, Status status, Throwable throwable, Metadata metadata) { return handleException(ctx, throwable, metadata, this::applyStatusProto); } diff --git a/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunction.java b/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunction.java index d1febb98819..6e9f150e7d0 100644 --- a/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunction.java +++ b/grpc/src/main/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunction.java @@ -51,13 +51,11 @@ static GrpcExceptionHandlerFunction of() { * Maps the specified {@link Throwable} to a gRPC {@link Status} and mutates the specified {@link Metadata}. * If {@code null} is returned, {@link #of()} will be used to return {@link Status} as the default. * - *

The {@link Status} may also be specified as a parameter if it is created by - * the upstream gRPC framework. - * You can return the {@link Status} or any other {@link Status} as needed. If the exception is raised - * internally in Armeria, no {@link Status} created, so {@code null} will be specified. + *

The specified {@link Status} parameter was created via {@link Status#fromThrowable(Throwable)}. + * You can return the {@link Status} or any other {@link Status} as needed. */ @Nullable - Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata); + Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata); /** * Returns a {@link GrpcExceptionHandlerFunction} that returns the result of this function diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java b/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java index 795ab184b36..a395d37fce7 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java @@ -17,6 +17,8 @@ import static com.linecorp.armeria.internal.client.ClientUtil.initContextAndExecuteWithFallback; import static com.linecorp.armeria.internal.client.grpc.protocol.InternalGrpcWebUtil.messageBuf; +import static com.linecorp.armeria.internal.common.grpc.GrpcExceptionHandlerFunctionUtil.fromThrowable; +import static com.linecorp.armeria.internal.common.grpc.GrpcExceptionHandlerFunctionUtil.generateMetadataFromThrowable; import static java.util.Objects.requireNonNull; import static java.util.concurrent.TimeUnit.MILLISECONDS; @@ -248,9 +250,14 @@ public void start(Listener responseListener, Metadata metadata) { prepareHeaders(compressor, metadata, remainingNanos); final BiFunction errorResponseFactory = - (unused, cause) -> HttpResponse.ofFailure(exceptionHandler.apply(ctx, null, cause, metadata) - .withDescription(cause.getMessage()) - .asRuntimeException()); + (unused, cause) -> { + final Metadata responseMetadata = generateMetadataFromThrowable(cause); + Status status = fromThrowable(ctx, exceptionHandler, cause, responseMetadata); + if (status.getDescription() == null) { + status = status.withDescription(cause.getMessage()); + } + return HttpResponse.ofFailure(status.asRuntimeException()); + }; final HttpResponse res = initContextAndExecuteWithFallback( httpClient, ctx, endpointGroup, HttpResponse::of, errorResponseFactory); @@ -453,8 +460,8 @@ public void onNext(DeframedMessage message) { } }); } catch (Throwable t) { - final Metadata metadata = new Metadata(); - close(exceptionHandler.apply(ctx, null, t, metadata), metadata); + final Metadata metadata = generateMetadataFromThrowable(t); + close(fromThrowable(ctx, exceptionHandler, t, metadata), metadata); } } @@ -510,8 +517,8 @@ private void prepareHeaders(Compressor compressor, Metadata metadata, long remai } private void closeWhenListenerThrows(Throwable t) { - final Metadata metadata = new Metadata(); - closeWhenEos(exceptionHandler.apply(ctx, null, t, metadata), metadata); + final Metadata metadata = generateMetadataFromThrowable(t); + closeWhenEos(fromThrowable(ctx, exceptionHandler, t, metadata), metadata); } private void closeWhenEos(Status status, Metadata metadata) { diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/UnwrappingGrpcExceptionHandleFunction.java b/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcExceptionHandlerFunctionUtil.java similarity index 51% rename from grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/UnwrappingGrpcExceptionHandleFunction.java rename to grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcExceptionHandlerFunctionUtil.java index 95bc1df782c..5a87733d532 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/UnwrappingGrpcExceptionHandleFunction.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcExceptionHandlerFunctionUtil.java @@ -13,13 +13,11 @@ * License for the specific language governing permissions and limitations * under the License. */ - package com.linecorp.armeria.internal.common.grpc; import static java.util.Objects.requireNonNull; import com.linecorp.armeria.common.RequestContext; -import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.grpc.GrpcExceptionHandlerFunction; import com.linecorp.armeria.common.grpc.protocol.ArmeriaStatusException; import com.linecorp.armeria.common.util.Exceptions; @@ -27,23 +25,36 @@ import io.grpc.Metadata; import io.grpc.Status; -public final class UnwrappingGrpcExceptionHandleFunction implements GrpcExceptionHandlerFunction { - private final GrpcExceptionHandlerFunction delegate; +public final class GrpcExceptionHandlerFunctionUtil { - public UnwrappingGrpcExceptionHandleFunction(GrpcExceptionHandlerFunction handlerFunction) { - delegate = handlerFunction; + public static Metadata generateMetadataFromThrowable(Throwable exception) { + final Metadata metadata = Status.trailersFromThrowable(peelAndUnwrap(exception)); + return metadata != null ? metadata : new Metadata(); } - @Nullable - @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { - final Throwable t = peelAndUnwrap(cause); - return delegate.apply(ctx, status, t, metadata); + public static Status fromThrowable(RequestContext ctx, GrpcExceptionHandlerFunction exceptionHandler, + Throwable t, Metadata metadata) { + final Status status = Status.fromThrowable(peelAndUnwrap(t)); + final Throwable cause = status.getCause(); + if (cause == null) { + return status; + } + return applyExceptionHandler(ctx, exceptionHandler, status, cause, metadata); + } + + public static Status applyExceptionHandler(RequestContext ctx, + GrpcExceptionHandlerFunction exceptionHandler, + Status status, Throwable cause, Metadata metadata) { + final Throwable peeled = peelAndUnwrap(cause); + status = exceptionHandler.apply(ctx, status, peeled, metadata); + assert status != null; + return status; } private static Throwable peelAndUnwrap(Throwable t) { requireNonNull(t, "t"); - Throwable cause = Exceptions.peel(t); + t = Exceptions.peel(t); + Throwable cause = t; while (cause != null) { if (cause instanceof ArmeriaStatusException) { return StatusExceptionConverter.toGrpc((ArmeriaStatusException) cause); @@ -52,4 +63,6 @@ private static Throwable peelAndUnwrap(Throwable t) { } return t; } + + private GrpcExceptionHandlerFunctionUtil() {} } diff --git a/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframer.java b/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframer.java index 44f3dfa4e3b..4fb84f701c6 100644 --- a/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframer.java +++ b/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframer.java @@ -17,6 +17,8 @@ package com.linecorp.armeria.internal.common.grpc; import static com.google.common.base.Preconditions.checkState; +import static com.linecorp.armeria.internal.common.grpc.GrpcExceptionHandlerFunctionUtil.fromThrowable; +import static com.linecorp.armeria.internal.common.grpc.GrpcExceptionHandlerFunctionUtil.generateMetadataFromThrowable; import static java.util.Objects.requireNonNull; import com.linecorp.armeria.common.HttpHeaderNames; @@ -119,9 +121,9 @@ public void processHeaders(HttpHeaders headers, StreamDecoderOutput void startCall( } } - private void startCall(ServerMethodDefinition methodDef, ServiceRequestContext ctx, - HttpRequest req, MethodDescriptor methodDescriptor, - AbstractServerCall call) { + private static void startCall(ServerMethodDefinition methodDef, ServiceRequestContext ctx, + HttpRequest req, MethodDescriptor methodDescriptor, + AbstractServerCall call) { final Listener listener; final Metadata headers = MetadataUtil.copyFromHeaders(req.headers()); try { @@ -320,9 +326,10 @@ private void startCall(ServerMethodDefinition methodDef, ServiceReq call.setListener(listener); call.startDeframing(); ctx.whenRequestCancelling().handle((cancellationCause, unused) -> { - final Status status = call.exceptionHandler().apply(ctx, null, cancellationCause, headers); - assert status != null; - call.close(new ServerStatusAndMetadata(status, new Metadata(), true, true)); + final Metadata metadata = generateMetadataFromThrowable(cancellationCause); + call.close(new ServerStatusAndMetadata( + fromThrowable(ctx, call.exceptionHandler(), cancellationCause, metadata), + metadata, true, true)); return null; }); } diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java index 017e9144778..b7d285b0a9d 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java @@ -51,7 +51,6 @@ import com.linecorp.armeria.common.grpc.GrpcStatusFunction; import com.linecorp.armeria.common.grpc.protocol.AbstractMessageDeframer; import com.linecorp.armeria.common.grpc.protocol.ArmeriaMessageFramer; -import com.linecorp.armeria.internal.common.grpc.UnwrappingGrpcExceptionHandleFunction; import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.HttpServiceWithRoutes; import com.linecorp.armeria.server.Server; @@ -1000,7 +999,7 @@ public GrpcService build() { registryBuilder.addService(grpcHealthCheckService.bindService(), null, ImmutableList.of()); } - GrpcExceptionHandlerFunction grpcExceptionHandler; + final GrpcExceptionHandlerFunction grpcExceptionHandler; if (exceptionMappingsBuilder != null) { grpcExceptionHandler = exceptionMappingsBuilder.build().orElse(GrpcExceptionHandlerFunction.of()); } else if (exceptionHandler != null) { @@ -1008,7 +1007,6 @@ public GrpcService build() { } else { grpcExceptionHandler = GrpcExceptionHandlerFunction.of(); } - grpcExceptionHandler = new UnwrappingGrpcExceptionHandleFunction(grpcExceptionHandler); registryBuilder.setDefaultExceptionHandler(grpcExceptionHandler); if (interceptors != null) { diff --git a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HandlerRegistry.java b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HandlerRegistry.java index 47b8d63e9c0..6044ef4c969 100644 --- a/grpc/src/main/java/com/linecorp/armeria/server/grpc/HandlerRegistry.java +++ b/grpc/src/main/java/com/linecorp/armeria/server/grpc/HandlerRegistry.java @@ -71,7 +71,6 @@ import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.grpc.GrpcExceptionHandlerFunction; import com.linecorp.armeria.internal.common.ReflectiveDependencyInjector; -import com.linecorp.armeria.internal.common.grpc.UnwrappingGrpcExceptionHandleFunction; import com.linecorp.armeria.internal.server.annotation.AnnotationUtil; import com.linecorp.armeria.internal.server.annotation.DecoratorAnnotationUtil; import com.linecorp.armeria.internal.server.annotation.DecoratorAnnotationUtil.DecoratorAndOrder; @@ -282,8 +281,7 @@ private static void putGrpcExceptionHandlerIfPresent( grpcExceptionHandler.ifPresent(exceptionHandler -> { GrpcExceptionHandlerFunction grpcExceptionHandler0 = exceptionHandler; if (defaultExceptionHandler != null) { - grpcExceptionHandler0 = new UnwrappingGrpcExceptionHandleFunction( - exceptionHandler.orElse(defaultExceptionHandler)); + grpcExceptionHandler0 = exceptionHandler.orElse(defaultExceptionHandler); } grpcExceptionHandlersBuilder.put(methodDefinition, grpcExceptionHandler0); }); diff --git a/grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientTest.java b/grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientTest.java index b48708f0e5b..1c0580317b2 100644 --- a/grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/client/grpc/GrpcClientTest.java @@ -747,7 +747,7 @@ void cancelAfterBegin() throws Exception { responseObserver.awaitCompletion(); assertThat(responseObserver.getValues()).isEmpty(); assertThat(GrpcExceptionHandlerFunction.of() - .apply(null, null, responseObserver.getError(), null) + .apply(null, Status.UNKNOWN, responseObserver.getError(), null) .getCode()).isEqualTo(Code.CANCELLED); final RequestLog log = requestLogQueue.take(); @@ -783,7 +783,7 @@ void cancelAfterFirstResponse() throws Exception { responseObserver.awaitCompletion(operationTimeoutMillis(), TimeUnit.MILLISECONDS); assertThat(responseObserver.getValues()).hasSize(1); assertThat(GrpcExceptionHandlerFunction.of() - .apply(null, null, responseObserver.getError(), null) + .apply(null, Status.UNKNOWN, responseObserver.getError(), null) .getCode()).isEqualTo(Code.CANCELLED); checkRequestLog((rpcReq, rpcRes, grpcStatus) -> { @@ -1418,7 +1418,7 @@ void deadlineExceededServerStreaming() throws Exception { assertThat(recorder.getError()).isNotNull(); assertThat(GrpcExceptionHandlerFunction.of() - .apply(null, null, recorder.getError(), null) + .apply(null, Status.UNKNOWN, recorder.getError(), null) .getCode()) .isEqualTo(Status.DEADLINE_EXCEEDED.getCode()); @@ -1618,10 +1618,10 @@ void statusCodeAndMessage() throws Exception { verify(responseObserver, timeout(operationTimeoutMillis())).onError(captor.capture()); assertThat(GrpcExceptionHandlerFunction.of() - .apply(null, null, captor.getValue(), null) + .apply(null, Status.UNKNOWN, captor.getValue(), null) .getCode()).isEqualTo(Status.UNKNOWN.getCode()); assertThat(GrpcExceptionHandlerFunction.of() - .apply(null, null, captor.getValue(), null) + .apply(null, Status.UNKNOWN, captor.getValue(), null) .getDescription()).isEqualTo(errorMessage); verifyNoMoreInteractions(responseObserver); diff --git a/grpc/src/test/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunctionTest.java b/grpc/src/test/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunctionTest.java index 9e4740b62bd..0287cd53de8 100644 --- a/grpc/src/test/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunctionTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/common/grpc/DefaultGrpcExceptionHandlerFunctionTest.java @@ -33,7 +33,8 @@ class DefaultGrpcExceptionHandlerFunctionTest { void failFastExceptionToUnavailableCode() { assertThat(GrpcExceptionHandlerFunction .of() - .apply(null, null, new FailFastException(CircuitBreaker.ofDefaultName()), null) + .apply(null, Status.UNKNOWN, new FailFastException(CircuitBreaker.ofDefaultName()), + null) .getCode()).isEqualTo(Status.Code.UNAVAILABLE); } @@ -41,7 +42,7 @@ void failFastExceptionToUnavailableCode() { void invalidProtocolBufferExceptionToInvalidArgumentCode() { assertThat(GrpcExceptionHandlerFunction .of() - .apply(null, null, + .apply(null, Status.UNKNOWN, new InvalidProtocolBufferException("Failed to parse message"), null) .getCode()).isEqualTo(Status.Code.INVALID_ARGUMENT); } diff --git a/grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunctionBuilderTest.java b/grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunctionBuilderTest.java index 9e81415a3c6..5d9e3b1de27 100644 --- a/grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunctionBuilderTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcExceptionHandlerFunctionBuilderTest.java @@ -89,19 +89,19 @@ void sortExceptionHandler() { final GrpcExceptionHandlerFunction exceptionHandler = builder.build().orElse( GrpcExceptionHandlerFunction.of()); - Status status = exceptionHandler.apply(ctx, null, new A3Exception(), new Metadata()); + Status status = exceptionHandler.apply(ctx, Status.UNKNOWN, new A3Exception(), new Metadata()); assertThat(status.getCode()).isEqualTo(Code.UNAUTHENTICATED); - status = exceptionHandler.apply(ctx, null, new A2Exception(), new Metadata()); + status = exceptionHandler.apply(ctx, Status.UNKNOWN, new A2Exception(), new Metadata()); assertThat(status.getCode()).isEqualTo(Code.UNIMPLEMENTED); - status = exceptionHandler.apply(ctx, null, new A1Exception(), new Metadata()); + status = exceptionHandler.apply(ctx, Status.UNKNOWN, new A1Exception(), new Metadata()); assertThat(status.getCode()).isEqualTo(Code.RESOURCE_EXHAUSTED); - status = exceptionHandler.apply(ctx, null, new B2Exception(), new Metadata()); + status = exceptionHandler.apply(ctx, Status.UNKNOWN, new B2Exception(), new Metadata()); assertThat(status.getCode()).isEqualTo(Code.NOT_FOUND); - status = exceptionHandler.apply(ctx, null, new B1Exception(), new Metadata()); + status = exceptionHandler.apply(ctx, Status.UNKNOWN, new B1Exception(), new Metadata()); assertThat(status.getCode()).isEqualTo(Code.UNAUTHENTICATED); } @@ -116,7 +116,7 @@ void mapStatus() { for (Throwable ex : ImmutableList.of(new A2Exception(), new A3Exception())) { final Metadata metadata = new Metadata(); - final Status newStatus = exceptionHandler.apply(ctx, null, ex, metadata); + final Status newStatus = exceptionHandler.apply(ctx, Status.UNKNOWN, ex, metadata); assertThat(newStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED); assertThat(newStatus.getCause()).isEqualTo(ex); assertThat(metadata.keys()).isEmpty(); @@ -124,7 +124,7 @@ void mapStatus() { final A1Exception cause = new A1Exception(); final Metadata metadata = new Metadata(); - final Status newStatus = exceptionHandler.apply(ctx, null, cause, metadata); + final Status newStatus = exceptionHandler.apply(ctx, Status.UNKNOWN, cause, metadata); assertThat(newStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED); assertThat(newStatus.getCause()).isEqualTo(cause); @@ -144,14 +144,14 @@ void mapStatusAndMetadata() { final B1Exception cause = new B1Exception(); final Metadata metadata1 = new Metadata(); - final Status newStatus1 = exceptionHandler.apply(ctx, null, cause, metadata1); + final Status newStatus1 = exceptionHandler.apply(ctx, Status.UNKNOWN, cause, metadata1); assertThat(newStatus1.getCode()).isEqualTo(Code.ABORTED); assertThat(metadata1.get(TEST_KEY)).isEqualTo("B1Exception"); assertThat(metadata1.keys()).containsOnly(TEST_KEY.name()); final Metadata metadata2 = new Metadata(); metadata2.put(TEST_KEY2, "test"); - final Status newStatus2 = exceptionHandler.apply(ctx, null, cause, metadata2); + final Status newStatus2 = exceptionHandler.apply(ctx, Status.UNKNOWN, cause, metadata2); assertThat(newStatus2.getCode()).isEqualTo(Code.ABORTED); assertThat(metadata2.get(TEST_KEY)).isEqualTo("B1Exception"); diff --git a/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframerTest.java b/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframerTest.java index d3ad2170874..5d68db290d4 100644 --- a/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframerTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/HttpStreamDeframerTest.java @@ -58,8 +58,7 @@ void setUp() { final ServiceRequestContext ctx = ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); final TransportStatusListener statusListener = (status, metadata) -> statusRef.set(status); deframer = new HttpStreamDeframer(DecompressorRegistry.getDefaultInstance(), ctx, statusListener, - new UnwrappingGrpcExceptionHandleFunction( - GrpcExceptionHandlerFunction.of()), Integer.MAX_VALUE, + GrpcExceptionHandlerFunction.of(), Integer.MAX_VALUE, false, true); } diff --git a/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/TestServiceImpl.java b/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/TestServiceImpl.java index f661dc3cc09..f4cd411c314 100644 --- a/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/TestServiceImpl.java +++ b/grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/TestServiceImpl.java @@ -367,7 +367,8 @@ private synchronized void dispatchChunk() { } catch (Throwable e) { failure = e; if (GrpcExceptionHandlerFunction.of() - .apply(ServiceRequestContext.current(), null, e, new Metadata()) + .apply(ServiceRequestContext.current(), Status.UNKNOWN, + e, new Metadata()) .getCode() == Status.CANCELLED.getCode()) { // Stream was cancelled by client, responseStream.onError() might be called already or // will be called soon by inbounding StreamObserver. diff --git a/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerAnnotationOnlyTest.java b/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerAnnotationOnlyTest.java index 07d794bcf57..fe6394647ec 100644 --- a/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerAnnotationOnlyTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerAnnotationOnlyTest.java @@ -134,7 +134,7 @@ private static class FirstGrpcExceptionHandler implements GrpcExceptionHandlerFu @Nullable @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { exceptionHandler.add("first"); if (Objects.equals(cause.getMessage(), "first")) { return Status.UNAUTHENTICATED; @@ -147,7 +147,7 @@ private static class SecondGrpcExceptionHandler implements GrpcExceptionHandler @Nullable @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { exceptionHandler.add("second"); if (Objects.equals(cause.getMessage(), "second")) { return Status.INVALID_ARGUMENT; diff --git a/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerFunctionUtilTest.java b/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerFunctionUtilTest.java new file mode 100644 index 00000000000..5d4e594799e --- /dev/null +++ b/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerFunctionUtilTest.java @@ -0,0 +1,94 @@ +/* + * 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.server.grpc; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.concurrent.CompletionException; + +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import com.linecorp.armeria.client.grpc.GrpcClients; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +import io.grpc.Status; +import io.grpc.StatusRuntimeException; +import io.grpc.stub.StreamObserver; +import testing.grpc.Messages.SimpleRequest; +import testing.grpc.Messages.SimpleRequest.NestedRequest; +import testing.grpc.Messages.SimpleResponse; +import testing.grpc.TestServiceGrpc.TestServiceBlockingStub; +import testing.grpc.TestServiceGrpc.TestServiceImplBase; + +class GrpcExceptionHandlerFunctionUtilTest { + + @RegisterExtension + static final ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) throws Exception { + sb.service(GrpcService.builder() + .addService(new TestServiceImpl()) + .exceptionHandler((ctx, status, throwable, metadata) -> { + assertThat(throwable).isInstanceOf(RuntimeException.class); + return Status.INTERNAL; + }) + .build()); + } + }; + + @CsvSource({ "onError", "throw", "onErrorStatus", "throwStatus" }) + @ParameterizedTest + void classAndMethodHaveMultipleExceptionHandlers(String exceptionType) { + final TestServiceBlockingStub client = + GrpcClients.newClient(server.httpUri(), TestServiceBlockingStub.class); + + final SimpleRequest globalRequest = + SimpleRequest.newBuilder() + .setNestedRequest(NestedRequest.newBuilder().setNestedPayload(exceptionType) + .build()) + .build(); + assertThatThrownBy(() -> client.unaryCall(globalRequest)) + .isInstanceOfSatisfying(StatusRuntimeException.class, + e -> assertThat(e.getStatus()).isEqualTo(Status.INTERNAL)); + } + + private static class TestServiceImpl extends TestServiceImplBase { + + @Override + public void unaryCall(SimpleRequest request, StreamObserver responseObserver) { + final CompletionException exception = new CompletionException(new RuntimeException()); + switch (request.getNestedRequest().getNestedPayload()) { + case "onError": + responseObserver.onError(exception); + break; + case "throw": + throw exception; + case "onErrorStatus": + responseObserver.onError(Status.INTERNAL.withCause(exception).asRuntimeException()); + break; + case "throwStatus": + throw Status.INTERNAL.withCause(exception).asRuntimeException(); + default: + throw new IllegalArgumentException("unknown payload"); + } + } + } +} diff --git a/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerTest.java b/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerTest.java index e398e731348..18f85dbf5ed 100644 --- a/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerTest.java +++ b/grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcExceptionHandlerTest.java @@ -470,7 +470,7 @@ private static class FirstGrpcExceptionHandler implements GrpcExceptionHandlerFu @Nullable @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { exceptionHandler.add("first"); if (Objects.equals(cause.getMessage(), "first")) { return Status.UNAUTHENTICATED; @@ -483,7 +483,7 @@ private static class SecondGrpcExceptionHandler implements GrpcExceptionHandlerF @Nullable @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { exceptionHandler.add("second"); if (Objects.equals(cause.getMessage(), "second")) { return Status.INVALID_ARGUMENT; @@ -496,7 +496,7 @@ private static class ThirdGrpcExceptionHandler implements GrpcExceptionHandlerFu @Nullable @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { exceptionHandler.add("third"); if (Objects.equals(cause.getMessage(), "third")) { return Status.NOT_FOUND; @@ -509,7 +509,7 @@ private static class ForthGrpcExceptionHandler implements GrpcExceptionHandlerFu @Nullable @Override - public Status apply(RequestContext ctx, @Nullable Status status, Throwable cause, Metadata metadata) { + public Status apply(RequestContext ctx, Status status, Throwable cause, Metadata metadata) { exceptionHandler.add("forth"); if (Objects.equals(cause.getMessage(), "forth")) { return Status.UNAVAILABLE;