From d6f7ae3415cdd335f406c4306a0d3f9182e9b6a3 Mon Sep 17 00:00:00 2001 From: chickenchickenlove Date: Thu, 7 Nov 2024 01:31:25 +0900 Subject: [PATCH] ClientRequestContext thorws a exception when host, authoriy is null. --- .../armeria/client/ClientRequestContext.java | 4 --- .../client/ClientRequestContextWrapper.java | 2 -- .../client/DefaultClientRequestContext.java | 31 ++++++++++++------- .../client/ClientRequestContextTest.java | 28 +++++++++++++++++ 4 files changed, 48 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java index 017197f2bde..833bce3d566 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java @@ -290,15 +290,11 @@ ClientRequestContext newDerivedContext(RequestId id, @Nullable HttpRequest req, *
  • {@link Endpoint#authority()}.
    • * */ - @Nullable - @UnstableApi String authority(); /** * Returns the host part of {@link #authority()}, without a port number. */ - @Nullable - @UnstableApi String host(); /** diff --git a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.java b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.java index f03e0a15de3..599e3dbfa2e 100644 --- a/core/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.java +++ b/core/src/main/java/com/linecorp/armeria/client/ClientRequestContextWrapper.java @@ -70,13 +70,11 @@ public String fragment() { return unwrap().fragment(); } - @Nullable @Override public String authority() { return unwrap().authority(); } - @Nullable @Override public String host() { return unwrap().host(); diff --git a/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java b/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java index 6af10aad221..4b58031eae0 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java +++ b/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java @@ -483,15 +483,20 @@ private void failEarly(Throwable cause) { // TODO(ikhoon): Consider moving the logic for filling authority to `HttpClientDelegate.exceute()`. private void autoFillSchemeAuthorityAndOrigin() { - final String authority = authority(); - if (authority != null && endpoint != null && endpoint.isIpAddrOnly()) { - // The connection will be established with the IP address but `host` set to the `Endpoint` - // could be used for SNI. It would make users send HTTPS requests with CSLB or configure a reverse - // proxy based on an authority. - final String host = SchemeAndAuthority.of(null, authority).host(); - if (!NetUtil.isValidIpV4Address(host) && !NetUtil.isValidIpV6Address(host)) { - endpoint = endpoint.withHost(host); + + try { + final String authority = authority(); + if (endpoint != null && endpoint.isIpAddrOnly()) { + // The connection will be established with the IP address but `host` set to the `Endpoint` + // could be used for SNI. It would make users send HTTPS requests + // with CSLB or configure a reverse proxy based on an authority. + final String host = SchemeAndAuthority.of(null, authority).host(); + if (!NetUtil.isValidIpV4Address(host) && !NetUtil.isValidIpV6Address(host)) { + endpoint = endpoint.withHost(host); + } } + } catch (IllegalStateException e) { + // Just pass, because it's normal condition. } final HttpHeadersBuilder headersBuilder = internalRequestHeaders.toBuilder(); @@ -750,7 +755,6 @@ public String fragment() { return requestTarget().fragment(); } - @Nullable @Override public String authority() { final HttpHeaders additionalRequestHeaders = this.additionalRequestHeaders; @@ -774,6 +778,11 @@ public String authority() { if (authority == null) { authority = internalRequestHeaders.get(HttpHeaderNames.HOST); } + if (authority == null) { + throw new IllegalStateException( + "ClientRequestContext may be in the process of initialization." + + "In this case, host() or authority() could be null"); + } return authority; } @@ -794,12 +803,12 @@ private String origin() { return origin; } - @Nullable @Override public String host() { final String authority = authority(); if (authority == null) { - return null; + throw new IllegalStateException("ClientRequestContext may be in the process of initialization." + + "In this case, host() or authority() could be null"); } return SchemeAndAuthority.of(null, authority).host(); } diff --git a/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java b/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java index d549701c386..8ed808021df 100644 --- a/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/ClientRequestContextTest.java @@ -18,6 +18,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.function.Function; import java.util.stream.Stream; @@ -29,6 +31,7 @@ import org.junit.jupiter.params.provider.ArgumentsSource; import org.junit.jupiter.params.provider.ValueSource; +import com.linecorp.armeria.common.AggregatedHttpResponse; import com.linecorp.armeria.common.HttpMethod; import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.RequestContext; @@ -269,6 +272,31 @@ void hasOwnAttr() { } } + @Test + void callAuthorityShouldBeThrownDuringPartiallyInit() { + final CountDownLatch countDownLatch = new CountDownLatch(1); + final WebClient client = WebClient + .builder("http://localhost:8080") + .contextCustomizer(ctx -> { + countDownLatch.countDown(); + assertThatThrownBy(ctx::host) + .isInstanceOf(IllegalStateException.class) + .hasMessage( + "ClientRequestContext may be in the process of initialization." + + "In this case, host() or authority() could be null"); + assertThatThrownBy(ctx::authority) + .isInstanceOf(IllegalStateException.class) + .hasMessage( + "ClientRequestContext may be in the process of initialization." + + "In this case, host() or authority() could be null" + ); + }).build(); + + final CompletableFuture aggregate = client.get("/").aggregate(); + assertThat(countDownLatch.getCount()).isEqualTo(0); + aggregate.cancel(true); + } + @ParameterizedTest @ValueSource(strings = {"%", "http:///", "http://foo.com/bar"}) void updateRequestWithInvalidPath(String path) {