From f66fec71a646448b37a0b1bdbc99f7eafd9da3bd Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 19 Nov 2024 10:07:39 +0000 Subject: [PATCH 01/27] Update public suffix list (#5991) 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 | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) 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 bb89032a551..c06ebd7f160 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -1682,7 +1682,6 @@ com.im com.in com.io com.iq -com.is com.jo com.kg com.ki @@ -2234,7 +2233,6 @@ edu.ht edu.in edu.io edu.iq -edu.is edu.it edu.jo edu.kg @@ -3176,7 +3174,6 @@ gov.in gov.io gov.iq gov.ir -gov.is gov.it gov.jo gov.kg @@ -3474,6 +3471,8 @@ herokussl.com heroy.more-og-romsdal.no heroy.nordland.no heteml.net +heyflow.page +heyflow.site hf.space hi.cn hi.us @@ -3821,7 +3820,6 @@ inf.ua infiniti info info.at -info.au info.az info.bb info.bj @@ -3874,7 +3872,6 @@ int.co int.cv int.eu.org int.in -int.is int.la int.lk int.mv @@ -5747,7 +5744,6 @@ net.in net.io net.iq net.ir -net.is net.je net.jo net.kg @@ -5961,7 +5957,6 @@ nohost.me noip.me noip.us nokia -nom.ad nom.ag nom.co nom.es @@ -6354,7 +6349,6 @@ org.in org.io org.iq org.ir -org.is org.je org.jo org.kg From d93b92c672d864f23a6c3f2e5c4d8072aa62c157 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 20 Nov 2024 10:06:59 +0000 Subject: [PATCH 02/27] Update public suffix list (#5993) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- .../com/linecorp/armeria/public_suffixes.txt | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-) 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 c06ebd7f160..efa51bfc933 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -346,7 +346,6 @@ aem.live aem.page aero aero.mv -aero.tt aerobatic.aero aeroclub.aero aerodrome.aero @@ -371,6 +370,7 @@ agency agents.aero agr.br agrar.hu +agri.jo agric.za agrigento.it agro.bj @@ -381,6 +381,7 @@ ah.cn ah.no ai ai.in +ai.jo ai.vn aibetsu.hokkaido.jp aichi.jp @@ -612,7 +613,6 @@ art.pl art.sn arte arte.bo -arts.co arts.nf arts.ro arts.ve @@ -915,7 +915,6 @@ bestbuy bet bet.ar bet.br -betainabox.com better-than.tv bf bg @@ -1800,7 +1799,6 @@ coop.mv coop.mw coop.py coop.rw -coop.tt cooperativa.bo copro.uk corsica @@ -2459,6 +2457,7 @@ enebakk.no energy enf.br eng.br +eng.jo eng.pro engerdal.no engine.aero @@ -2662,7 +2661,6 @@ firewall-gateway.com firewall-gateway.de firewall-gateway.net firewalledreplit.co -firm.co firm.dk firm.ht firm.in @@ -2708,6 +2706,7 @@ fly.dev fm fm.br fm.it +fm.jo fm.no fnc.fr-par.scw.cloud fnd.br @@ -3824,7 +3823,6 @@ info.az info.bb info.bj info.bo -info.co info.cx info.ec info.et @@ -3868,7 +3866,6 @@ int.ar int.az int.bo int.ci -int.co int.cv int.eu.org int.in @@ -3880,7 +3877,6 @@ int.ni int.pt int.ru int.tj -int.tt int.ve int.vn international @@ -4128,7 +4124,6 @@ jnj jo joboji.iwate.jp jobs -jobs.tt joburg joetsu.niigata.jp jogasz.hu @@ -5201,6 +5196,7 @@ mil.tj mil.tm mil.to mil.tr +mil.tt mil.tw mil.tz mil.uy @@ -5328,7 +5324,6 @@ mobi mobi.gp mobi.ke mobi.ng -mobi.tt mobi.tz mobile mochizuki.nagano.jp @@ -5440,7 +5435,6 @@ museum museum.mv museum.no museum.om -museum.tt music musica.ar musica.bo @@ -5602,7 +5596,6 @@ name.eg name.et name.fj name.hr -name.jo name.mk name.mv name.my @@ -5969,7 +5962,6 @@ nom.ni nom.pa nom.pe nom.pl -nom.re nom.ro nom.tm nom.ve @@ -6596,9 +6588,9 @@ peewee.jp penne.jp penza.su pepper.jp +per.jo per.la per.nf -per.sg perma.jp perso.ht perso.sn @@ -6621,6 +6613,7 @@ pharmacien.fr pharmaciens.km pharmacy phd +phd.jo philips phone photo @@ -6913,7 +6906,6 @@ realtor realty rebun.hokkaido.jp rec.br -rec.co rec.nf rec.ro rec.ve @@ -8487,7 +8479,6 @@ trapani.it travel travel.in travel.pl -travel.tt travelers travelersinsurance travinh.vn @@ -8592,6 +8583,7 @@ tv.br tv.im tv.in tv.it +tv.jo tv.sd tv.tr tv.tz @@ -8977,7 +8969,6 @@ weather weatherchannel web.app web.bo -web.co web.do web.gu web.id From 6dbed576fe8f0774f445716c74c01c7572799ee6 Mon Sep 17 00:00:00 2001 From: Yizhou Feng <66976730+yzfeng2020@users.noreply.github.com> Date: Wed, 20 Nov 2024 20:58:38 -0800 Subject: [PATCH 03/27] Introduce AccessLogWriterUtil and Remove Unnecessary RequestContext Push/Pop (#5985) Motivation: As described in https://github.com/line/armeria/issues/5984, there are instances of unnecessary request context push/pop operations related to access logging when the access log writer is disabled. These operations are redundant since the logging path does not need to be triggered when the writer is disabled. Optimizing this behavior reduces unnecessary overhead in such cases. Modifications: - Added `AccessLogWriterUtil#maybeWriteAccessLog` - Writes access log only when `TransientServiceOption#WITH_ACCESS_LOGGING` is enabled and the access log writer is not disabled. - Updated `AbstractHttpResponseHandler` and `HttpServerHandler` Result: - Closes https://github.com/line/armeria/issues/5984. - Removes unnecessary context push/pop operations for access logging when the access log writer is disabled, resulting in a more efficient request handling process. --- .../server/AbstractHttpResponseHandler.java | 18 +--- .../AbstractHttpResponseSubscriber.java | 7 +- .../armeria/server/AccessLogWriterUtil.java | 53 +++++++++ .../server/AggregatedHttpResponseHandler.java | 7 +- .../armeria/server/HttpServerHandler.java | 10 +- .../server/logging/AccessLogWriter.java | 3 +- .../logging/AccessLoggerIntegrationTest.java | 101 ++++++++++++++---- 7 files changed, 149 insertions(+), 50 deletions(-) create mode 100644 core/src/main/java/com/linecorp/armeria/server/AccessLogWriterUtil.java diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java b/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java index e49e5b8a120..1d2fb516b19 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 LINE Corporation + * 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 @@ -36,7 +36,6 @@ import com.linecorp.armeria.common.logging.RequestLogBuilder; import com.linecorp.armeria.common.logging.RequestLogProperty; import com.linecorp.armeria.common.stream.ClosedStreamException; -import com.linecorp.armeria.common.util.SafeCloseable; import com.linecorp.armeria.internal.common.CancellationScheduler.CancellationTask; import com.linecorp.armeria.internal.server.DefaultServiceRequestContext; @@ -221,21 +220,6 @@ final void endLogRequestAndResponse(@Nullable Throwable cause) { } } - /** - * Writes an access log if the {@link TransientServiceOption#WITH_ACCESS_LOGGING} option is enabled for - * the {@link #service()}. - */ - final void maybeWriteAccessLog() { - final ServiceConfig config = reqCtx.config(); - if (config.transientServiceOptions().contains(TransientServiceOption.WITH_ACCESS_LOGGING)) { - reqCtx.log().whenComplete().thenAccept(log -> { - try (SafeCloseable ignored = reqCtx.push()) { - config.accessLogWriter().log(log); - } - }); - } - } - /** * Schedules a request timeout. */ diff --git a/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java b/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java index 97ab6156740..e674102286a 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 LINE Corporation + * 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 @@ -18,6 +18,7 @@ import static com.google.common.base.MoreObjects.firstNonNull; import static com.linecorp.armeria.internal.common.HttpHeadersUtil.mergeTrailers; +import static com.linecorp.armeria.server.AccessLogWriterUtil.maybeWriteAccessLog; import java.nio.channels.ClosedChannelException; import java.util.concurrent.CompletableFuture; @@ -339,7 +340,7 @@ private void succeed() { cause = requestLog.responseCause(); } endLogRequestAndResponse(cause); - maybeWriteAccessLog(); + maybeWriteAccessLog(reqCtx); } } @@ -348,7 +349,7 @@ void fail(Throwable cause) { if (tryComplete(cause)) { setDone(true); endLogRequestAndResponse(cause); - maybeWriteAccessLog(); + maybeWriteAccessLog(reqCtx); } } diff --git a/core/src/main/java/com/linecorp/armeria/server/AccessLogWriterUtil.java b/core/src/main/java/com/linecorp/armeria/server/AccessLogWriterUtil.java new file mode 100644 index 00000000000..fdc1306644c --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/server/AccessLogWriterUtil.java @@ -0,0 +1,53 @@ +/* + * 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; + +import com.linecorp.armeria.common.util.SafeCloseable; +import com.linecorp.armeria.server.logging.AccessLogWriter; + +/** + * Utility class for {@link AccessLogWriter}. + */ +final class AccessLogWriterUtil { + + /** + * Writes an access log if the {@link TransientServiceOption#WITH_ACCESS_LOGGING} option is enabled + * for the {@link ServiceConfig#transientServiceOptions()} and the {@link ServiceConfig#accessLogWriter()} + * is not {@link AccessLogWriter#disabled()} for the given {@link ServiceRequestContext#config()}. + */ + static void maybeWriteAccessLog(ServiceRequestContext reqCtx) { + final ServiceConfig config = reqCtx.config(); + if (shouldWriteAccessLog(config)) { + reqCtx.log().whenComplete().thenAccept(log -> { + try (SafeCloseable ignored = reqCtx.push()) { + config.accessLogWriter().log(log); + } + }); + } + } + + /** + * Returns whether an access log should be written. + * + */ + private static boolean shouldWriteAccessLog(ServiceConfig config) { + return config.accessLogWriter() != AccessLogWriter.disabled() && + config.transientServiceOptions().contains(TransientServiceOption.WITH_ACCESS_LOGGING); + } + + private AccessLogWriterUtil() {} +} diff --git a/core/src/main/java/com/linecorp/armeria/server/AggregatedHttpResponseHandler.java b/core/src/main/java/com/linecorp/armeria/server/AggregatedHttpResponseHandler.java index ec59df678e8..d6e5657d54d 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AggregatedHttpResponseHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/AggregatedHttpResponseHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2021 LINE Corporation + * 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 @@ -17,6 +17,7 @@ package com.linecorp.armeria.server; import static com.google.common.base.MoreObjects.firstNonNull; +import static com.linecorp.armeria.server.AccessLogWriterUtil.maybeWriteAccessLog; import java.nio.channels.ClosedChannelException; import java.util.concurrent.CompletableFuture; @@ -125,7 +126,7 @@ private void recoverAndWrite(Throwable cause) { void fail(Throwable cause) { if (tryComplete(cause)) { endLogRequestAndResponse(cause); - maybeWriteAccessLog(); + maybeWriteAccessLog(reqCtx); } } @@ -185,7 +186,7 @@ void handleWriteComplete(ChannelFuture future, boolean isSuccess, @Nullable Thro } } endLogRequestAndResponse(cause); - maybeWriteAccessLog(); + maybeWriteAccessLog(reqCtx); } return; } diff --git a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java index f3722de3e91..c0584cf73e5 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 LINE Corporation + * 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 @@ -13,6 +13,7 @@ * License for the specific language governing permissions and limitations * under the License. */ + package com.linecorp.armeria.server; import static com.google.common.base.MoreObjects.firstNonNull; @@ -22,6 +23,7 @@ import static com.linecorp.armeria.common.SessionProtocol.H2C; import static com.linecorp.armeria.internal.common.HttpHeadersUtil.CLOSE_STRING; import static com.linecorp.armeria.internal.common.RequestContextUtil.NOOP_CONTEXT_HOOK; +import static com.linecorp.armeria.server.AccessLogWriterUtil.maybeWriteAccessLog; import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_WINDOW_SIZE; import static java.util.Objects.requireNonNull; @@ -612,11 +614,7 @@ private ChannelFuture respond(ServiceRequestContext reqCtx, ResponseHeadersBuild logBuilder.endResponse(firstNonNull(cause, f.cause())); } } - reqCtx.log().whenComplete().thenAccept(log -> { - try (SafeCloseable ignored = reqCtx.push()) { - reqCtx.config().accessLogWriter().log(log); - } - }); + maybeWriteAccessLog(reqCtx); }); return future; } diff --git a/core/src/main/java/com/linecorp/armeria/server/logging/AccessLogWriter.java b/core/src/main/java/com/linecorp/armeria/server/logging/AccessLogWriter.java index ff2b94d1cc6..c4f34ce77a3 100644 --- a/core/src/main/java/com/linecorp/armeria/server/logging/AccessLogWriter.java +++ b/core/src/main/java/com/linecorp/armeria/server/logging/AccessLogWriter.java @@ -1,5 +1,5 @@ /* - * Copyright 2018 LINE Corporation + * 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 @@ -13,6 +13,7 @@ * License for the specific language governing permissions and limitations * under the License. */ + package com.linecorp.armeria.server.logging; import static com.google.common.base.Preconditions.checkArgument; diff --git a/core/src/test/java/com/linecorp/armeria/server/logging/AccessLoggerIntegrationTest.java b/core/src/test/java/com/linecorp/armeria/server/logging/AccessLoggerIntegrationTest.java index bfc33e1dfe2..c451974526d 100644 --- a/core/src/test/java/com/linecorp/armeria/server/logging/AccessLoggerIntegrationTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/logging/AccessLoggerIntegrationTest.java @@ -19,48 +19,109 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.RequestContext; -import com.linecorp.armeria.common.logging.RequestLog; +import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.ServerBuilder; import com.linecorp.armeria.server.ServiceRequestContext; +import com.linecorp.armeria.server.TransientHttpService; +import com.linecorp.armeria.server.TransientServiceOption; import com.linecorp.armeria.testing.junit5.server.ServerExtension; class AccessLoggerIntegrationTest { - private static final AtomicReference CTX_REF = new AtomicReference<>(); + private static final AtomicReference REQUEST_CONTEXT_REFERENCE = new AtomicReference<>(); + private static final AtomicInteger CONTEXT_HOOK_COUNTER = new AtomicInteger(0); + + private static final AccessLogWriter ACCESS_LOG_WRITER = log -> + REQUEST_CONTEXT_REFERENCE.set(RequestContext.currentOrNull()); + + private static final Supplier CONTENT_HOOK = () -> { + CONTEXT_HOOK_COUNTER.incrementAndGet(); + return () -> {}; + }; + + private static final HttpService BASE_SERVICE = ((HttpService) (ctx, req) -> HttpResponse.of(200)) + .decorate((delegate, ctx, req) -> { + ctx.hook(CONTENT_HOOK); + return delegate.serve(ctx, req); + }); @RegisterExtension - static ServerExtension server = new ServerExtension() { + static final ServerExtension server = new ServerExtension() { @Override - protected void configure(ServerBuilder sb) throws Exception { - sb.service("/", (ctx, req) -> HttpResponse.of(200)); - sb.accessLogWriter(new AccessLogWriter() { - @Override - public void log(RequestLog log) { - CTX_REF.set(RequestContext.currentOrNull()); - } - }, false); + protected void configure(ServerBuilder sb) { + sb.route().path("/default-service") + .build(BASE_SERVICE); + sb.route().path("/default-service-with-access-log-writer") + .accessLogWriter(ACCESS_LOG_WRITER, false) + .build(BASE_SERVICE); + sb.route().path("/transit-service") + .build(BASE_SERVICE.decorate(TransientHttpService.newDecorator())); + sb.route().path("/transit-service-with-access-logger") + .accessLogWriter(ACCESS_LOG_WRITER, false) + .build(BASE_SERVICE.decorate(TransientHttpService.newDecorator())); + sb.route().path("/transit-service-with-access-log-option") + .build(BASE_SERVICE.decorate( + TransientHttpService.newDecorator(TransientServiceOption.WITH_ACCESS_LOGGING)) + ); + sb.route().path("/transit-service-with-access-log-option-and-access-logger") + .accessLogWriter(ACCESS_LOG_WRITER, false) + .build(BASE_SERVICE.decorate( + TransientHttpService.newDecorator(TransientServiceOption.WITH_ACCESS_LOGGING)) + ); } }; @BeforeEach - void beforeEach() { - CTX_REF.set(null); + void resetState() { + REQUEST_CONTEXT_REFERENCE.set(null); + CONTEXT_HOOK_COUNTER.set(0); } - @Test - void testAccessLogger() throws Exception { - assertThat(server.blockingWebClient().get("/").status().code()).isEqualTo(200); - assertThat(server.requestContextCaptor().size()).isEqualTo(1); + @CsvSource({ + "/default-service, false", + "/default-service-with-access-log-writer, true", + "/transit-service, false", + "/transit-service-with-access-logger, false", + "/transit-service-with-access-log-option, false", + "/transit-service-with-access-log-option-and-access-logger, true" + }) + @ParameterizedTest + void testAccessLogger(String path, boolean shouldWriteAccessLog) throws Exception { + assertThat(server.blockingWebClient().get(path).status().code()) + .as("Response status for path: %s", path) + .isEqualTo(200); + + assertThat(server.requestContextCaptor().size()) + .as("Expected exactly one captured context for path: %s", path) + .isEqualTo(1); + final ServiceRequestContext ctx = server.requestContextCaptor().poll(); - assertThat(ctx).isNotNull(); - await().untilAsserted(() -> assertThat(CTX_REF).hasValue(ctx)); + assertThat(ctx) + .as("ServiceRequestContext should not be null for path: %s", path) + .isNotNull(); + + if (shouldWriteAccessLog) { + await().untilAsserted(() -> + assertThat(REQUEST_CONTEXT_REFERENCE) + .as("Expected request context to be set for path: %s", path) + .hasValue(ctx) + ); + } + + final int expectedHookCounter = shouldWriteAccessLog ? 1 : 0; + assertThat(CONTEXT_HOOK_COUNTER) + .as("Context hook counter mismatch for path: %s", path) + .hasValue(expectedHookCounter); } } From 79112f537eb851aa2aba3b534777a608f2c71e0d Mon Sep 17 00:00:00 2001 From: jrhee17 Date: Thu, 21 Nov 2024 14:01:25 +0900 Subject: [PATCH 04/27] Fix race condition where `log.whenComplete` may not complete (#5986) Motivation: It has been reported that `log.whenComplete` is not completing in some cases in #5981. The cause seems to be a race condition between `DefaultRequestLog#endRequest` and `DefaultRequestLog#requestContent`. Completion of `log.whenComplete` is important because 1) it is semantically bound to an HTTP request 2) users (including us) have clean up logic using `log.whenComplete`. I propose that simply `endRequest` only sets `name` if content is not deferred, and `requestContent` sets `name` if content is deferred. The logic is easier to reason about, and there are minimal performance implications since a lock is held anyways. Modifications: - `#endRequest` sets `name` if `requestContent` isn't deferred - `#requestContent` sets name if `requestContent` is deferred Result: - Closes #5981 --- .../common/logging/DefaultRequestLog.java | 13 ++++------- .../common/logging/RequestLogBuilder.java | 4 ++++ .../common/logging/DefaultRequestLogTest.java | 22 +++++++++++++++++++ 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java b/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java index 7cd223bb086..250eed2fae3 100644 --- a/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java +++ b/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java @@ -992,11 +992,7 @@ public void requestContent(@Nullable Object requestContent, @Nullable Object raw ctx.updateRpcRequest((RpcRequest) requestContent); } updateFlags(RequestLogProperty.REQUEST_CONTENT); - - final int requestCompletionFlags = RequestLogProperty.FLAGS_REQUEST_COMPLETE & ~deferredFlags; - if (isAvailable(requestCompletionFlags)) { - setNamesIfAbsent(); - } + setNamesIfAbsent(); } @Nullable @@ -1104,12 +1100,11 @@ private void endRequest0(@Nullable Throwable requestCause, long requestEndTimeNa } } - // Set names if request content is not deferred or it was deferred but has been set - // before the request completion. - if (!hasInterestedFlags(deferredFlags, RequestLogProperty.REQUEST_CONTENT) || - isAvailable(RequestLogProperty.REQUEST_CONTENT)) { + // Set names if request content is not deferred + if (!hasInterestedFlags(deferredFlags, RequestLogProperty.REQUEST_CONTENT)) { setNamesIfAbsent(); } + this.requestEndTimeNanos = requestEndTimeNanos; if (requestCause instanceof HttpStatusException || requestCause instanceof HttpResponseException) { diff --git a/core/src/main/java/com/linecorp/armeria/common/logging/RequestLogBuilder.java b/core/src/main/java/com/linecorp/armeria/common/logging/RequestLogBuilder.java index 6b1badfb1a9..61915cfaaa6 100644 --- a/core/src/main/java/com/linecorp/armeria/common/logging/RequestLogBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/common/logging/RequestLogBuilder.java @@ -109,12 +109,16 @@ void session(@Nullable Channel channel, SessionProtocol sessionProtocol, @Nullab *
  • A path pattern and HTTP method name for {@link HttpService}
    • * * This property is often used as a meter tag or distributed trace's span name. + * Note that calling {@link #responseContent(Object, Object)} will automatically fill + * {@link RequestLogProperty#NAME}, so {@link #name(String, String)} must be called beforehand. */ void name(String serviceName, String name); /** * Sets the human-readable name of the {@link Request}, such as RPC method name, annotated service method * name or HTTP method name. This property is often used as a meter tag or distributed trace's span name. + * Note that calling {@link #responseContent(Object, Object)} will automatically fill + * {@link RequestLogProperty#NAME}, so {@link #name(String)} must be called beforehand. */ void name(String name); diff --git a/core/src/test/java/com/linecorp/armeria/common/logging/DefaultRequestLogTest.java b/core/src/test/java/com/linecorp/armeria/common/logging/DefaultRequestLogTest.java index fb95c5bff37..dfc8838b61d 100644 --- a/core/src/test/java/com/linecorp/armeria/common/logging/DefaultRequestLogTest.java +++ b/core/src/test/java/com/linecorp/armeria/common/logging/DefaultRequestLogTest.java @@ -28,6 +28,9 @@ import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.BlockingQueue; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BiFunction; import org.junit.jupiter.api.BeforeEach; @@ -49,6 +52,7 @@ import com.linecorp.armeria.common.RpcResponse; import com.linecorp.armeria.common.SerializationFormat; import com.linecorp.armeria.common.SessionProtocol; +import com.linecorp.armeria.common.util.ThreadFactories; import com.linecorp.armeria.internal.testing.AnticipatedException; import com.linecorp.armeria.internal.testing.ImmediateEventLoop; import com.linecorp.armeria.server.HttpService; @@ -544,4 +548,22 @@ void testPendingLogsAlwaysInEventLoop() { .satisfiesAnyOf(t0 -> assertThat(t0).isEqualTo(testThread), t0 -> assertThat(ctx.eventLoop().inEventLoop(t0)).isTrue())); } + + @Test + void nameIsAlwaysSet() { + final AtomicInteger atomicInteger = new AtomicInteger(); + final ExecutorService executorService = + Executors.newFixedThreadPool(2, ThreadFactories.newThreadFactory("test", true)); + // a heurestic number of iterations to reproduce #5981 + final int numIterations = 1000; + for (int i = 0; i < numIterations; i++) { + final ServiceRequestContext sctx = ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); + final DefaultRequestLog log = new DefaultRequestLog(sctx); + log.defer(RequestLogProperty.REQUEST_CONTENT); + executorService.execute(log::endRequest); + executorService.execute(() -> log.requestContent(null, null)); + log.whenRequestComplete().thenRun(atomicInteger::incrementAndGet); + } + await().untilAsserted(() -> assertThat(atomicInteger).hasValue(numIterations)); + } } From 6af3a6c9b89762f8e4d3b8a425f33b52c86652a3 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Thu, 21 Nov 2024 14:49:51 +0900 Subject: [PATCH 05/27] Fix Jacoco failure (#5994) Motivation: ``` Caused by: java.lang.IllegalStateException: Can't add different class with same name: com/linecorp/armeria/common/logback/LoggingEventWrapper at org.jacoco.core.analysis.CoverageBuilder.visitCoverage(CoverageBuilder.java:106) at org.jacoco.core.analysis.Analyzer$1.visitEnd(Analyzer.java:100) at org.objectweb.asm.ClassVisitor.visitEnd(ClassVisitor.java:395) at org.jacoco.core.internal.flow.ClassProbesAdapter.visitEnd(ClassProbesAdapter.java:100) at org.objectweb.asm.ClassReader.accept(ClassReader.java:749) at org.objectweb.asm.ClassReader.accept(ClassReader.java:425) at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:117) at org.jacoco.core.analysis.Analyzer.analyzeClass(Analyzer.java:133) ... 152 more ``` Modifications: - Set `no_aggregation` flag to `:it:logback1.5` Result: Fixes #5989 --- settings.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/settings.gradle b/settings.gradle index e5ecc716ad6..64a00b3b1af 100644 --- a/settings.gradle +++ b/settings.gradle @@ -219,7 +219,7 @@ includeWithFlags ':it:jackson-provider', 'java', 'relocate includeWithFlags ':it:kotlin', 'java', 'relocate', 'kotlin' includeWithFlags ':it:kubernetes-chaos-tests', 'java', 'relocate' includeWithFlags ':it:logback1.4', 'java11', 'relocate' -includeWithFlags ':it:logback1.5', 'java11', 'relocate' +includeWithFlags ':it:logback1.5', 'java11', 'relocate', 'no_aggregation' includeWithFlags ':it:multipart', 'java17', 'relocate' includeWithFlags ':it:nio', 'java', 'relocate' includeWithFlags ':it:okhttp', 'java', 'relocate' From 86f78668a4c4dace573725e5ed3dfb1c03b8c7c1 Mon Sep 17 00:00:00 2001 From: jrhee17 Date: Thu, 21 Nov 2024 15:38:51 +0900 Subject: [PATCH 06/27] Exclude protobuf 4.x from `armeria-grpc` module (#5992) Motivation: With the recent release of 1.31.0, we received a report that protobuf 4 has been included as an api dependency. This is probably a mistake since 1) the community isn't ready for protobuf 4 2) it's usually safer to follow the protobuf version used by `grpc-java`. The `api` configuration is a consumable configuration, which means it is difficult to detect these kind of issues directly. However, by checking the runtime dependencies for tests we can infer whether a version is inadvertently bumped. In order to detect such mishaps, I also propose that a `failOnVersionConflict` variant is added. Because naively introducing `failOnVersionConflict` introduces many conflicts, I've added a variant which checks for specified dependencies only. (inspired by https://github.com/gradle/gradle/issues/8813) One limitation is that the `dependencies` task is not available when a `failOnVersionConflict` occurs. For this reason, once a failure due to a conflict occurs, it is encouraged to use the `-PdebugDeps` flag with the `dependencies` task. e.g. ``` ./gradlew :grpc:dependencies -PdebugDeps ``` Modifications: - Excluded the `protobuf-java` dependency from `protobuf-jackson` - Added a `failOnVersionConflict` method which checks version conflicts for specific dependencies only Result: - Closes #5990 --- build.gradle | 4 +++ gradle/scripts/.gitrepo | 6 ++-- gradle/scripts/lib/common-dependencies.gradle | 34 +++++++++++++++++++ gradle/scripts/lib/java-shade.gradle | 31 +++++++++++++---- grpc/build.gradle | 4 ++- 5 files changed, 68 insertions(+), 11 deletions(-) diff --git a/build.gradle b/build.gradle index ccb39c888fc..c63e12570d5 100644 --- a/build.gradle +++ b/build.gradle @@ -489,3 +489,7 @@ allprojects { } } } + +configure(projectsWithFlags('java', 'publish')) { + failOnVersionConflict(libs.protobuf.java) +} diff --git a/gradle/scripts/.gitrepo b/gradle/scripts/.gitrepo index d770e09d55f..fc59a047a16 100644 --- a/gradle/scripts/.gitrepo +++ b/gradle/scripts/.gitrepo @@ -6,7 +6,7 @@ [subrepo] remote = https://github.com/line/gradle-scripts branch = main - commit = 1f94acd56f170782ad291e4603384ad59cca4e9e - parent = d18437e44118f1367ce3cf2d7e5008552ebd7513 + commit = 597bb9e29378d56051db3ace62d5cbd81f3b1272 + parent = 7da456555cd58b9fe3cd387a7fe1003be7504411 method = merge - cmdver = 0.4.5 + cmdver = 0.4.6 diff --git a/gradle/scripts/lib/common-dependencies.gradle b/gradle/scripts/lib/common-dependencies.gradle index 19eaf73c5b2..12046f0781d 100644 --- a/gradle/scripts/lib/common-dependencies.gradle +++ b/gradle/scripts/lib/common-dependencies.gradle @@ -21,6 +21,7 @@ allprojects { p -> managedVersions = getManagedVersions(p.rootProject) findLibrary = this.&findLibrary.curry(p.rootProject) findPlugin = this.&findPlugin.curry(p.rootProject) + failOnVersionConflict = this.&failOnVersionConflict.curry(p) } } @@ -331,3 +332,36 @@ final class GentlePlainTextReporter implements Reporter { return delegate.getFileExtension() } } + +/** + * A custom version of failOnVersionConflict which can limit which dependencies should be checked for conflict. + * Heavily inspired by https://github.com/gradle/gradle/issues/8813. + */ +static def failOnVersionConflict(Project project, ProviderConvertible providerConvertible) { + return failOnVersionConflict(project, providerConvertible.asProvider()) +} + +static def failOnVersionConflict(Project project, Provider dependencyProvider) { + if (!dependencyProvider.isPresent()) { + return + } + def targetDependency = dependencyProvider.get() + project.configurations.configureEach { config -> + incoming.afterResolve { + resolutionResult.allComponents {ResolvedComponentResult result -> + if (selectionReason.conflictResolution && moduleVersion != null) { + // we don't care if the selected version is the one specified in dependencies.toml + if (targetDependency.module == moduleVersion.module && targetDependency.version != moduleVersion.version) { + def msg = "Project '${project.name}:${config.name}' resolution failed " + + "for '${targetDependency.module}' with '${getSelectionReason()}" + if (project.rootProject.hasProperty('debugDeps')) { + project.logger.lifecycle(msg) + } else { + throw new IllegalStateException(msg) + } + } + } + } + } + } +} diff --git a/gradle/scripts/lib/java-shade.gradle b/gradle/scripts/lib/java-shade.gradle index 33dd4b17a70..fd1ddd3ec55 100644 --- a/gradle/scripts/lib/java-shade.gradle +++ b/gradle/scripts/lib/java-shade.gradle @@ -6,7 +6,7 @@ import java.util.concurrent.atomic.AtomicInteger buildscript { repositories { gradlePluginPortal() - mavenCentral() + google() } dependencies { classpath "gradle.plugin.com.github.johnrengelman:shadow:${managedVersions['gradle.plugin.com.github.johnrengelman:shadow']}" @@ -88,7 +88,6 @@ configure(relocatedProjects) { group: 'Build', description: 'Extracts the shaded test JAR.', dependsOn: tasks.shadedTestJar) { - from(zipTree(tasks.shadedTestJar.archiveFile.get().asFile)) from(sourceSets.test.output.classesDirs) { // Add the JAR resources excluded in the 'shadedTestJar' task. @@ -335,7 +334,8 @@ private void configureShadowTask(Project project, ShadowJar task, boolean isMain private Configuration configureShadedTestImplementConfiguration( Project project, Project recursedProject = project, Set excludeRules = new HashSet<>(), - Set visitedProjects = new HashSet<>()) { + Set visitedProjects = new HashSet<>(), + boolean recursedProjectRelocated = true) { def shadedJarTestImplementation = project.configurations.getByName('shadedJarTestImplementation') @@ -364,14 +364,24 @@ private Configuration configureShadedTestImplementConfiguration( }.each { cfg -> cfg.allDependencies.each { dep -> if (dep instanceof ProjectDependency) { + if (!dep.dependencyProject.hasFlag('java')) { + // Do not add the dependencies of non-Java projects. + return + } // Project dependency - recurse later. // Note that we recurse later to have immediate module dependencies higher precedence. projectDependencies.add(dep) } else { // Module dependency - add. if (shadedDependencyNames.contains("${dep.group}:${dep.name}")) { - // Skip the shaded dependencies. - return + if (recursedProjectRelocated) { + // Skip the shaded dependencies. + return + } + throw new IllegalStateException( + "${recursedProject} has a shaded dependency: ${dep.group}:${dep.name} " + + "but it is not relocated. Please add a 'relocate' flag to " + + "${recursedProject} in settings.gradle.") } if (excludeRules.find { rule -> @@ -382,16 +392,23 @@ private Configuration configureShadedTestImplementConfiguration( } // Do not use `project.dependencies.add(name, dep)` that discards the classifier of // a dependency. See https://github.com/gradle/gradle/issues/23096 - project.configurations.getByName(shadedJarTestImplementation.name).dependencies.add(dep) + shadedJarTestImplementation.dependencies.add(dep) } } } // Recurse into the project dependencies. projectDependencies.each { ProjectDependency dep -> + if (!dep.dependencyProject.hasFlag('relocate')) { + shadedJarTestImplementation.dependencies.add( + project.dependencies.project(path: dep.dependencyProject.path)) + recursedProjectRelocated = false + } else { + recursedProjectRelocated = true + } configureShadedTestImplementConfiguration( project, dep.dependencyProject, - excludeRules + dep.excludeRules, visitedProjects) + excludeRules + dep.excludeRules, visitedProjects, recursedProjectRelocated) } return shadedJarTestImplementation diff --git a/grpc/build.gradle b/grpc/build.gradle index b898b81065e..93c750457d9 100644 --- a/grpc/build.gradle +++ b/grpc/build.gradle @@ -15,7 +15,9 @@ dependencies { optionalImplementation libs.grpc.kotlin optionalImplementation libs.kotlin.coroutines.core - api libs.protobuf.jackson + api(libs.protobuf.jackson) { + exclude group: 'com.google.protobuf', module: 'protobuf-java' + } testImplementation(libs.gax.grpc) { exclude group: 'com.google.protobuf', module: 'protobuf-java' From dc99553e9b7bef544a77a03fd5eb3cd7e2367788 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 21 Nov 2024 10:06:58 +0000 Subject: [PATCH 07/27] Update public suffix list (#5996) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- .../src/main/resources/com/linecorp/armeria/public_suffixes.txt | 2 +- 1 file changed, 1 insertion(+), 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 efa51bfc933..1d7eb01bb65 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -1545,6 +1545,7 @@ co.com co.cr co.cz co.dk +co.dm co.education co.events co.financial @@ -8330,7 +8331,6 @@ tm.fr tm.hu tm.km tm.mc -tm.mg tm.no tm.pl tm.ro From ffe68f47a7358e1db12b3d357ac8d983731a6625 Mon Sep 17 00:00:00 2001 From: jrhee17 Date: Fri, 22 Nov 2024 11:27:15 +0900 Subject: [PATCH 08/27] Release notes for 1.31.1 (#5995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ![Screenshot 2024-11-21 at 14-47-09 1 31 1 release notes — Armeria release notes](https://github.com/user-attachments/assets/a3804abd-08ad-466a-987d-ac375952c43f) --- dependencies.toml | 5 ++++- grpc/build.gradle | 4 +--- site/src/pages/release-notes/1.31.1.mdx | 25 +++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 site/src/pages/release-notes/1.31.1.mdx diff --git a/dependencies.toml b/dependencies.toml index 5f34cd69717..a0b8b740fcf 100644 --- a/dependencies.toml +++ b/dependencies.toml @@ -992,7 +992,10 @@ version.ref = "protobuf-gradle-plugin" [libraries.protobuf-jackson] module = "org.curioswitch.curiostack:protobuf-jackson" version.ref = "protobuf-jackson" -exclusions = "javax.annotation:javax.annotation-api" +exclusions = [ + 'com.google.protobuf:protobuf-java', + "javax.annotation:javax.annotation-api", +] javadocs = "https://developers.curioswitch.org/apidocs/java/" [libraries.reactor-core] diff --git a/grpc/build.gradle b/grpc/build.gradle index 93c750457d9..b898b81065e 100644 --- a/grpc/build.gradle +++ b/grpc/build.gradle @@ -15,9 +15,7 @@ dependencies { optionalImplementation libs.grpc.kotlin optionalImplementation libs.kotlin.coroutines.core - api(libs.protobuf.jackson) { - exclude group: 'com.google.protobuf', module: 'protobuf-java' - } + api libs.protobuf.jackson testImplementation(libs.gax.grpc) { exclude group: 'com.google.protobuf', module: 'protobuf-java' diff --git a/site/src/pages/release-notes/1.31.1.mdx b/site/src/pages/release-notes/1.31.1.mdx new file mode 100644 index 00000000000..fc86f1298be --- /dev/null +++ b/site/src/pages/release-notes/1.31.1.mdx @@ -0,0 +1,25 @@ +--- +date: 2024-11-22 +--- + +## 🛠️ Bug fixes + +- Fixed a race condition which intermittently prevented from completing. #5981 #5986 +- `armeria-grpc` no longer exposes `protobuf-java` 4.x as a compile-time dependency. #5990 #5992 + +## 📈 Improvements + +- Slightly improved performance when is used. #5984 #5985 + +## 🙇 Thank you + + \ No newline at end of file From d531fe24d4511c22eb62062e13eef51ff172dc34 Mon Sep 17 00:00:00 2001 From: minux Date: Fri, 22 Nov 2024 14:41:10 +0900 Subject: [PATCH 09/27] Add the release note for 1.30.2 (#5998) --- site/src/pages/release-notes/1.30.2.mdx | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 site/src/pages/release-notes/1.30.2.mdx diff --git a/site/src/pages/release-notes/1.30.2.mdx b/site/src/pages/release-notes/1.30.2.mdx new file mode 100644 index 00000000000..223bc9d325b --- /dev/null +++ b/site/src/pages/release-notes/1.30.2.mdx @@ -0,0 +1,20 @@ +--- +date: 2024-11-22 +--- + +## 🛠️ Bug fixes + +- Fixed a race condition which intermittently prevented from completing. #5981 #5986 +- Fixed a bug where the original request path is not exposed to . #5931 #5932 + - You can access the raw request path via . +- Fixed a bug where the is unnecessarily pushed and popped. #5985 + +## 🙇 Thank you + + From 40beab1425c8d6f72e68e0ef3a68087d1d19ac1c Mon Sep 17 00:00:00 2001 From: Armeria Date: Fri, 22 Nov 2024 05:51:04 +0000 Subject: [PATCH 10/27] Update the project version to 1.31.2-SNAPSHOT --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index d943c0e1f96..7760cf281fe 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ group=com.linecorp.armeria -version=1.31.1-SNAPSHOT +version=1.31.2-SNAPSHOT projectName=Armeria projectUrl=https://armeria.dev/ projectDescription=Asynchronous HTTP/2 RPC/REST client/server library built on top of Java 8, Netty, Thrift and gRPC From c4e8fed525d5a0d12824c7c008e3314dc6c911a6 Mon Sep 17 00:00:00 2001 From: minux Date: Fri, 22 Nov 2024 18:24:08 +0900 Subject: [PATCH 11/27] Add manual button for publishing site (#6000) --- .github/workflows/publish-site.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.github/workflows/publish-site.yml b/.github/workflows/publish-site.yml index 67e34bcb5b7..9311acb3874 100644 --- a/.github/workflows/publish-site.yml +++ b/.github/workflows/publish-site.yml @@ -1,6 +1,12 @@ name: Publish Armeria site on: + workflow_dispatch: + inputs: + version: + description: 'Release Version' + required: true + type: string push: tags: - armeria-* @@ -32,7 +38,11 @@ jobs: - name: Build the site run: | - ./gradlew --no-daemon --stacktrace --max-workers=2 --parallel -PgithubToken=${{ secrets.GITHUB_TOKEN }} site + if [ "${{ github.event_name }}" == "workflow_dispatch" ]; then + ./gradlew --no-daemon --stacktrace --max-workers=2 --parallel -PgithubToken=${{ secrets.GITHUB_TOKEN }} -Pversion=${{ inputs.version }} site + else + ./gradlew --no-daemon --stacktrace --max-workers=2 --parallel -PgithubToken=${{ secrets.GITHUB_TOKEN }} site + fi - name: Deploy the site uses: peaceiris/actions-gh-pages@v4 From 9bb5215572e73f5cb7c89e0e713b5e4a9635dd5a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 23 Nov 2024 10:06:56 +0000 Subject: [PATCH 12/27] Update public suffix list (#6001) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- .../src/main/resources/com/linecorp/armeria/public_suffixes.txt | 2 +- 1 file changed, 1 insertion(+), 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 1d7eb01bb65..4b9681c4446 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -5161,7 +5161,6 @@ mil.do mil.ec mil.eg mil.fj -mil.ge mil.gh mil.gt mil.hn @@ -7543,6 +7542,7 @@ schmidt schokokeks.net scholarships school +school.ge school.nz school.za schoolbus.jp From 7a6ddd7250ab9d6fe6c39d7ac5be7aa22350de33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B9=80=ED=83=9C=ED=9B=88?= <46879264+Bue-von-hon@users.noreply.github.com> Date: Mon, 25 Nov 2024 10:31:42 +0900 Subject: [PATCH 13/27] Automatically register the metics of `PooledByteBufAllocator.DEFAULT` (#5916) Motivation: Add metrics related to PooledByteBufAllocator that is already exposed by the netty. Modifications: - `NettyAllocatorMetrics` are registered when MoreMeterBinders is initialized. Result: - Fixes #2633 - Armeria now automatically registers the metics of `PooledByteBufAllocator.DEFAULT`. --------- Co-authored-by: Ikhun Um --- .../linecorp/armeria/common/metric/MoreMeterBinders.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/src/main/java/com/linecorp/armeria/common/metric/MoreMeterBinders.java b/core/src/main/java/com/linecorp/armeria/common/metric/MoreMeterBinders.java index 1e569e408cc..4c82f8d993c 100644 --- a/core/src/main/java/com/linecorp/armeria/common/metric/MoreMeterBinders.java +++ b/core/src/main/java/com/linecorp/armeria/common/metric/MoreMeterBinders.java @@ -25,10 +25,13 @@ import com.google.common.collect.ImmutableList; +import com.linecorp.armeria.common.Flags; import com.linecorp.armeria.common.annotation.UnstableApi; import com.linecorp.armeria.internal.common.util.CertificateUtil; import io.micrometer.core.instrument.binder.MeterBinder; +import io.micrometer.core.instrument.binder.netty4.NettyAllocatorMetrics; +import io.netty.buffer.PooledByteBufAllocator; import io.netty.channel.EventLoopGroup; /** @@ -36,6 +39,12 @@ */ public final class MoreMeterBinders { + static { + // Bind the default Netty allocator metrics to the default MeterRegistry. + new NettyAllocatorMetrics(PooledByteBufAllocator.DEFAULT) + .bindTo(Flags.meterRegistry()); + } + /** * Returns a new {@link MeterBinder} to observe Netty's {@link EventLoopGroup}s. The following stats are * currently exported per registered {@link MeterIdPrefix}. From 08f6a0b7cafb5a8776ea9469a6ff06e679358cfc Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 27 Nov 2024 10:07:21 +0000 Subject: [PATCH 14/27] Update public suffix list (#6004) 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 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 4b9681c4446..2818d8a84ef 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -3699,6 +3699,7 @@ icu icurus.jp id id.au +id.cv id.firewalledreplit.co id.forgerock.io id.ir @@ -5707,6 +5708,7 @@ net.cm net.cn net.co net.cu +net.cv net.cw net.cy net.dm @@ -6804,6 +6806,7 @@ pu.it pub pub.instances.scw.cloud pub.sa +publ.cv publ.pt public-inquiry.uk pubtls.org @@ -9252,7 +9255,6 @@ xn--clchc0ea0b2g2a9gcd xn--czr694b xn--czrs0t xn--czru2d -xn--czrw28b.tw xn--d1acj3b xn--d1alf xn--d1at.xn--90a3ac @@ -9553,7 +9555,6 @@ xn--trna-woa.no xn--troms-zua.no xn--tysvr-vra.no xn--uc0atv.hk -xn--uc0atv.tw xn--uc0atv.xn--j6w193g xn--uc0ay4a.hk xn--uist22h.jp @@ -9598,7 +9599,6 @@ xn--ygarden-p1a.no xn--ygbi2ammx xn--ystre-slidre-ujb.no xn--zbx025d.jp -xn--zf0ao64a.tw xn--zf0avx.hk xn--zfr164b xnbay.com From 6963c926a71be33d79bbca6bd6ac9e5ac3f52802 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Thu, 28 Nov 2024 14:44:07 +0900 Subject: [PATCH 15/27] Fill the stack trace of `ResponseCompleteException` when sampled (#5972) Motivation: `ResponseCompleteException` is considered a safe exception for cleaning up request resources. However, it would be a good idea to leave a stack trace in case there is a bug in the implementation or the user wants to know how the error occurred. Discord thread: https://discord.com/channels/1087271586832318494/1087272728177942629/1303562249629073520 Modifications: - Create a new instance if `ResponseCompletionException` is sampled by `verboseExceptionSampler` Result: You can now sample the stack trace of `ResponseCompleteException` with `verboseExceptionSampler`. --- .../armeria/common/ResponseCompleteException.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/ResponseCompleteException.java b/core/src/main/java/com/linecorp/armeria/common/ResponseCompleteException.java index 5f767a94064..04035b2d132 100644 --- a/core/src/main/java/com/linecorp/armeria/common/ResponseCompleteException.java +++ b/core/src/main/java/com/linecorp/armeria/common/ResponseCompleteException.java @@ -26,16 +26,22 @@ public final class ResponseCompleteException extends CancellationException { private static final long serialVersionUID = 6090278381004263949L; - private static final ResponseCompleteException INSTANCE = new ResponseCompleteException(); + private static final ResponseCompleteException INSTANCE = new ResponseCompleteException(false); /** * Returns the singleton {@link ResponseCompleteException}. */ public static ResponseCompleteException get() { - return INSTANCE; + if (Flags.verboseExceptionSampler().isSampled(ResponseCompleteException.class)) { + return new ResponseCompleteException(); + } else { + return INSTANCE; + } } - private ResponseCompleteException() { + private ResponseCompleteException() {} + + private ResponseCompleteException(@SuppressWarnings("unused") boolean dummy) { super(null, null, false, false); } } From 2e58501b89cf36539570bdb4275b43bbba7b6136 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Thu, 28 Nov 2024 14:52:07 +0900 Subject: [PATCH 16/27] Fix flaky ReactiveWebServerLoadBalancerInteropTest (#5999) Motivation: ``` com.linecorp.armeria.spring.web.reactive.ReactiveWebServerLoadBalancerInteropTest.[1] sessionProtocol=H1C, path=/controller/api/ping java.util.ConcurrentModificationException at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1714) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682) at com.linecorp.armeria.spring.web.reactive.ReactiveWebServerLoadBalancerInteropTest.assertNoErrorLogByHttpWebHandlerAdapter(ReactiveWebServerLoadBalancerInteropTest.java:148) ``` It seems that `logAppender.list` was modified while retrieving elements Modifications: - Use `CopyOnWriteArrayList` to collect `ILoggingEvent` Result: Fixes #5462 --- ...ReactiveWebServerLoadBalancerInteropTest.java | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/spring/boot3-webflux-autoconfigure/src/test/java/com/linecorp/armeria/spring/web/reactive/ReactiveWebServerLoadBalancerInteropTest.java b/spring/boot3-webflux-autoconfigure/src/test/java/com/linecorp/armeria/spring/web/reactive/ReactiveWebServerLoadBalancerInteropTest.java index 48c50893de5..84b5d2d3856 100644 --- a/spring/boot3-webflux-autoconfigure/src/test/java/com/linecorp/armeria/spring/web/reactive/ReactiveWebServerLoadBalancerInteropTest.java +++ b/spring/boot3-webflux-autoconfigure/src/test/java/com/linecorp/armeria/spring/web/reactive/ReactiveWebServerLoadBalancerInteropTest.java @@ -22,6 +22,8 @@ import static org.springframework.web.reactive.function.server.RouterFunctions.route; import static org.springframework.web.reactive.function.server.ServerResponse.ok; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.stream.Collectors; import org.junit.jupiter.api.AfterEach; @@ -50,7 +52,7 @@ import ch.qos.logback.classic.Logger; import ch.qos.logback.classic.spi.ILoggingEvent; -import ch.qos.logback.core.read.ListAppender; +import ch.qos.logback.core.AppenderBase; import reactor.core.publisher.Mono; @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) @@ -79,7 +81,7 @@ RouterFunction routerFunction() { int port; final Logger httpWebHandlerAdapterLogger = (Logger) LoggerFactory.getLogger(HttpWebHandlerAdapter.class); - final ListAppender logAppender = new ListAppender<>(); + final ConcurrentListAppender logAppender = new ConcurrentListAppender<>(); @BeforeEach public void attachAppender() { @@ -148,4 +150,14 @@ private void assertNoErrorLogByHttpWebHandlerAdapter() { .collect(Collectors.toList())) .isEmpty(); } + + private static final class ConcurrentListAppender extends AppenderBase { + + List list = new CopyOnWriteArrayList<>(); + + @Override + protected void append(E eventObject) { + list.add(eventObject); + } + } } From dd7873231d1ee373bc6e99dc103d17d1209b2391 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 28 Nov 2024 10:07:32 +0000 Subject: [PATCH 17/27] Update public suffix list (#6005) 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 | 8 ++++++++ 1 file changed, 8 insertions(+) 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 2818d8a84ef..1c65e5f181a 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -267,6 +267,7 @@ ac.ci ac.cn ac.cr ac.cy +ac.eg ac.fj ac.gn ac.gov.br @@ -3827,6 +3828,7 @@ info.bj info.bo info.cx info.ec +info.eg info.et info.fj info.gu @@ -3891,6 +3893,7 @@ investments inzai.chiba.jp io io.in +io.noc.ruhr-uni-bochum.de io.vn iobb.net iopsys.se @@ -5049,6 +5052,7 @@ md md.us me me-south-1.elasticbeanstalk.com +me.eg me.eu.org me.in me.it @@ -7059,8 +7063,10 @@ ru ru.com ru.eu.org ru.net +rub.de rugby ruhr +ruhr-uni-bochum.de rulez.jp run runcontainers.dev @@ -7936,6 +7942,7 @@ sphinx.mythic-beasts.com spjelkavik.no spock.replit.dev sport +sport.eg sport.hu spot spydeberg.no @@ -8583,6 +8590,7 @@ tv tv.bb tv.bo tv.br +tv.eg tv.im tv.in tv.it From db289396f9fa218c56e503226cde09218760a40d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 29 Nov 2024 10:07:57 +0000 Subject: [PATCH 18/27] Update public suffix list (#6006) 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 1c65e5f181a..309b45eee2d 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -5046,7 +5046,6 @@ mc.it mcdir.me mcdir.ru mckinsey -mcpe.me mcpre.ru md md.us From 2134872f4c5325329cb731182e5f7d71e27517dc Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 3 Dec 2024 18:14:54 +0900 Subject: [PATCH 19/27] Fix a bug where stale endpoints in `KubernetesEndpointGroup` aren't updated (#6012) Motivation: When a Kubernetes service is updated, a new Pod watcher is created and receives events. The old cache should have been cleared at this point, but it was not removed, so stale endpoints were exposed. Modifications: - Clear `podToNode` map when a new Pod watcher is created Result: Fixed a bug where stale endpoints remained when a Kubernetes service was updated. --- .../endpoints/KubernetesEndpointGroup.java | 4 + ...KubernetesEndpointGroupMockServerTest.java | 77 +++++++++++++++++-- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/kubernetes/src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java b/kubernetes/src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java index cdaeac87c8d..3cd08c9c1ed 100644 --- a/kubernetes/src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java +++ b/kubernetes/src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java @@ -269,6 +269,8 @@ public void eventReceived(Action action, Service service0) { if (podWatch0 != null) { podWatch0.close(); } + // Clear the podToNode map before starting a new pod watch. + podToNode.clear(); podWatch0 = watchPod(service0.getSpec().getSelector()); if (closed) { podWatch0.close(); @@ -384,6 +386,8 @@ public void eventReceived(Action action, Node node) { nodeToIp.remove(nodeName); break; } + // TODO(ikhoon): Reschedule the update after a certain delay since multiple websocket events + // are updated in a same task. maybeUpdateEndpoints(); } diff --git a/kubernetes/src/test/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroupMockServerTest.java b/kubernetes/src/test/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroupMockServerTest.java index b1bbae90299..4ea74410dfe 100644 --- a/kubernetes/src/test/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroupMockServerTest.java +++ b/kubernetes/src/test/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroupMockServerTest.java @@ -162,6 +162,63 @@ void createEndpointsWithNodeIpAndPort() throws InterruptedException { }); } + @Test + void clearOldEndpointsWhenServiceIsUpdated() throws InterruptedException { + // Prepare Kubernetes resources + final List nodes = ImmutableList.of(newNode("1.1.1.1"), newNode("2.2.2.2"), newNode("3.3.3.3")); + final Deployment deployment = newDeployment(); + final int nodePort = 30000; + final Service service = newService(nodePort); + final List pods = nodes.stream() + .map(node -> node.getMetadata().getName()) + .map(nodeName -> newPod(deployment.getSpec().getTemplate(), nodeName)) + .collect(toImmutableList()); + + // Create Kubernetes resources + for (Node node : nodes) { + client.nodes().resource(node).create(); + } + client.pods().resource(pods.get(0)).create(); + client.pods().resource(pods.get(1)).create(); + client.apps().deployments().resource(deployment).create(); + client.services().resource(service).create(); + + final KubernetesEndpointGroup endpointGroup = KubernetesEndpointGroup.of(client, "test", + "nginx-service"); + endpointGroup.whenReady().join(); + + // Initial state + await().untilAsserted(() -> { + final List endpoints = endpointGroup.endpoints(); + // Wait until all endpoints are ready + assertThat(endpoints).containsExactlyInAnyOrder( + Endpoint.of("1.1.1.1", nodePort), + Endpoint.of("2.2.2.2", nodePort) + ); + }); + + // Update service and deployment with new selector + final int newNodePort = 30001; + final String newSelectorName = "nginx-updated"; + final Service updatedService = newService(newNodePort, newSelectorName); + client.services().resource(updatedService).update(); + final Deployment updatedDeployment = newDeployment(newSelectorName); + client.apps().deployments().resource(updatedDeployment).update(); + + final List updatedPods = + nodes.stream() + .map(node -> node.getMetadata().getName()) + .map(nodeName -> newPod(updatedDeployment.getSpec().getTemplate(), nodeName)) + .collect(toImmutableList()); + client.pods().resource(updatedPods.get(2)).create(); + await().untilAsserted(() -> { + final List endpoints = endpointGroup.endpoints(); + assertThat(endpoints).containsExactlyInAnyOrder( + Endpoint.of("3.3.3.3", newNodePort) + ); + }); + } + @Test void shouldUsePortNameToGetNodePort() { final List nodes = ImmutableList.of(newNode("1.1.1.1"), newNode("2.2.2.2"), newNode("3.3.3.3")); @@ -292,6 +349,10 @@ private static Node newNode(String ip) { } static Service newService(@Nullable Integer nodePort) { + return newService(nodePort, "nginx"); + } + + static Service newService(@Nullable Integer nodePort, String selectorName) { final ObjectMeta metadata = new ObjectMetaBuilder() .withName("nginx-service") .build(); @@ -301,7 +362,7 @@ static Service newService(@Nullable Integer nodePort) { .build(); final ServiceSpec serviceSpec = new ServiceSpecBuilder() .withPorts(servicePort) - .withSelector(ImmutableMap.of("app", "nginx")) + .withSelector(ImmutableMap.of("app", selectorName)) .withType("NodePort") .build(); return new ServiceBuilder() @@ -310,17 +371,17 @@ static Service newService(@Nullable Integer nodePort) { .build(); } - static Deployment newDeployment() { + static Deployment newDeployment(String selectorName) { final ObjectMeta metadata = new ObjectMetaBuilder() .withName("nginx-deployment") .build(); final LabelSelector selector = new LabelSelectorBuilder() - .withMatchLabels(ImmutableMap.of("app", "nginx")) + .withMatchLabels(ImmutableMap.of("app", selectorName)) .build(); final DeploymentSpec deploymentSpec = new DeploymentSpecBuilder() .withReplicas(4) .withSelector(selector) - .withTemplate(newPodTemplate()) + .withTemplate(newPodTemplate(selectorName)) .build(); return new DeploymentBuilder() .withMetadata(metadata) @@ -328,9 +389,13 @@ static Deployment newDeployment() { .build(); } - private static PodTemplateSpec newPodTemplate() { + static Deployment newDeployment() { + return newDeployment("nginx"); + } + + private static PodTemplateSpec newPodTemplate(String selectorName) { final ObjectMeta metadata = new ObjectMetaBuilder() - .withLabels(ImmutableMap.of("app", "nginx")) + .withLabels(ImmutableMap.of("app", selectorName)) .build(); final Container container = new ContainerBuilder() .withName("nginx") From 04e3a2a3f8462808267c797b4bf2ef4923bcf110 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 3 Dec 2024 18:55:25 +0900 Subject: [PATCH 20/27] Expose the default `HealthCheckUpdateHandler` (#6002) Motivation: When implementing a custom `HealthCheckUpdateHandler`, I just wanted to add validation in front of the default implementation. However, it wasn't possible because the default implementation has package-private visibility. Modifications: - Expose the default `HealthCheckUpdateHandler` via `HealthCheckUpdateHandler.of()` Result: You can now use the default `HealthCheckUpdateHandler` via `HealthCheckUpdateHandler.of()` --------- Co-authored-by: minux --- .../HealthCheckServiceBuilder.java | 3 ++- .../healthcheck/HealthCheckUpdateHandler.java | 25 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java b/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java index 72ac5c8e2f4..853847adbf8 100644 --- a/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckServiceBuilder.java @@ -234,10 +234,11 @@ public HealthCheckServiceBuilder longPolling(long maxLongPollingTimeoutMillis, * * @return {@code this} * @see #updatable(HealthCheckUpdateHandler) + * @see HealthCheckUpdateHandler#of() */ public HealthCheckServiceBuilder updatable(boolean updatable) { if (updatable) { - return updatable(DefaultHealthCheckUpdateHandler.INSTANCE); + return updatable(HealthCheckUpdateHandler.of()); } updateHandler = null; diff --git a/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckUpdateHandler.java b/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckUpdateHandler.java index 28c7ac54114..1625e42dff3 100644 --- a/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckUpdateHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckUpdateHandler.java @@ -18,6 +18,7 @@ import java.util.concurrent.CompletionStage; import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.annotation.UnstableApi; import com.linecorp.armeria.server.HttpResponseException; import com.linecorp.armeria.server.HttpStatusException; import com.linecorp.armeria.server.Server; @@ -28,6 +29,30 @@ */ @FunctionalInterface public interface HealthCheckUpdateHandler { + + /** + * Returns the default {@link HealthCheckUpdateHandler} that accepts a JSON object which has a boolean + * property named {@code "healthy"} for a {@code PUT} or {@code POST} request. A JSON patch in a + * {@code PATCH} request is also accepted. + * + *

    For example: + * 

    {@code
+     * // Update healthiness of the server to unhealthy
+     * POST /internal/health HTTP/2.0
+     *
+     * { "healthy": false }
+     *
+     * // Patch healthiness of the server to unhealthy
+     * PATCH /internal/health HTTP/2.0
+     *
+     * [ { "op": "replace", "path": "/healthy", "value": false } ]
+     * }
    + */ + @UnstableApi + static HealthCheckUpdateHandler of() { + return DefaultHealthCheckUpdateHandler.INSTANCE; + } + /** * Determines if the healthiness of the {@link Server} needs to be changed or not from the given * {@link HttpRequest}. From 5d346b242f2c63c8e462547e6dad213355bcfd10 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:07:32 +0000 Subject: [PATCH 21/27] Update public suffix list (#6015) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- .../src/main/resources/com/linecorp/armeria/public_suffixes.txt | 2 +- 1 file changed, 1 insertion(+), 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 309b45eee2d..f87101724ae 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -6096,6 +6096,7 @@ nz.eu.org o.bg o.se o0o0.jp +o365.cloud.nospamproxy.com oamishirasato.chiba.jp oarai.ibaraki.jp obama.fukui.jp @@ -8687,7 +8688,6 @@ uol uonuma.niigata.jp uozu.toyama.jp up.in -upli.io upow.gov.pl upper.jp uppo.gov.pl From 62da203bd9e11d669476e6999937352749cd7339 Mon Sep 17 00:00:00 2001 From: minux Date: Tue, 3 Dec 2024 21:03:14 +0900 Subject: [PATCH 22/27] Fix Bug Where Weight Is Set to 0 in Ramping-Up Strategy (#6014) Motivation: When the original weight is less than 10, the initial weight is incorrectly set to 0, potentially leading to `EndpointSelectionTimeoutException`. Modifications: - Ensured a minimum weight of 1 is set when the original weight is greater than 1 in the ramping-up strategy. - Added debugging logs for selector and selection strategy to facilitate troubleshooting. Result: - Fix a bug where weights could unintentionally be set to 0 in ramping-up strategies. --- .../client/endpoint/DynamicEndpointGroup.java | 39 ++++++++++-- .../client/endpoint/RoundRobinStrategy.java | 9 +++ .../StickyEndpointSelectionStrategy.java | 9 ++- .../endpoint/WeightRampingUpStrategy.java | 39 ++++++++++-- .../WeightRampingUpStrategyBuilder.java | 14 ++++- .../endpoint/WeightedRoundRobinStrategy.java | 35 +++++++++++ .../HealthCheckedEndpointGroup.java | 2 +- .../common/metric/MeterIdPrefixFunction.java | 1 + .../client/endpoint/EndpointToStringUtil.java | 61 +++++++++++++++++++ .../WeightedRandomDistributionSelector.java | 12 ++++ .../endpoint/WeightRampingUpStrategyTest.java | 1 - 11 files changed, 206 insertions(+), 16 deletions(-) create mode 100644 core/src/main/java/com/linecorp/armeria/internal/client/endpoint/EndpointToStringUtil.java diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/DynamicEndpointGroup.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/DynamicEndpointGroup.java index e28d7fbdced..9d630d05be5 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/DynamicEndpointGroup.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/DynamicEndpointGroup.java @@ -17,6 +17,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.linecorp.armeria.internal.client.endpoint.EndpointToStringUtil.toShortString; import static com.linecorp.armeria.internal.common.util.CollectionUtil.truncate; import static java.util.Objects.requireNonNull; @@ -32,6 +33,9 @@ import java.util.concurrent.locks.Lock; import java.util.function.Consumer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; @@ -51,6 +55,8 @@ */ public class DynamicEndpointGroup extends AbstractEndpointGroup implements ListenableAsyncCloseable { + private static final Logger logger = LoggerFactory.getLogger(DynamicEndpointGroup.class); + /** * Returns a newly created builder. */ @@ -223,6 +229,8 @@ protected final void addEndpoint(Endpoint e) { final List newEndpointsUnsorted = Lists.newArrayList(endpoints); newEndpointsUnsorted.add(e); endpoints = newEndpoints = ImmutableList.sortedCopyOf(newEndpointsUnsorted); + logger.info("An endpoint has been added: {}. Current endpoints: {}", + toShortString(e), toShortString(newEndpoints)); } finally { endpointsLock.unlock(); } @@ -238,12 +246,17 @@ protected final void removeEndpoint(Endpoint e) { final List newEndpoints; endpointsLock.lock(); try { - if (!allowEmptyEndpoints && endpoints.size() == 1) { + final List oldEndpoints = endpoints; + if (!allowEmptyEndpoints && oldEndpoints.size() == 1) { return; } - endpoints = newEndpoints = endpoints.stream() - .filter(endpoint -> !endpoint.equals(e)) - .collect(toImmutableList()); + endpoints = newEndpoints = oldEndpoints.stream() + .filter(endpoint -> !endpoint.equals(e)) + .collect(toImmutableList()); + if (endpoints.size() != oldEndpoints.size()) { + logger.info("An endpoint has been removed: {}. Current endpoints: {}", + toShortString(e), toShortString(newEndpoints)); + } } finally { endpointsLock.unlock(); } @@ -266,6 +279,7 @@ protected final void setEndpoints(Iterable endpoints) { return; } this.endpoints = newEndpoints; + logger.info("New endpoints have been set: {}", toShortString(newEndpoints)); } finally { endpointsLock.unlock(); } @@ -376,7 +390,7 @@ public String toString() { protected final String toString(Consumer builderMutator) { final StringBuilder buf = new StringBuilder(); buf.append(getClass().getSimpleName()); - buf.append("{selectionStrategy=").append(selectionStrategy.getClass()); + buf.append("{selector=").append(toStringSelector()); buf.append(", allowsEmptyEndpoints=").append(allowEmptyEndpoints); buf.append(", initialized=").append(initialEndpointsFuture.isDone()); buf.append(", numEndpoints=").append(endpoints.size()); @@ -385,6 +399,21 @@ protected final String toString(Consumer builderMutator) return buf.append('}').toString(); } + /** + * Returns the string representation of the {@link EndpointSelector} of this {@link DynamicEndpointGroup}. + * If the {@link EndpointSelector} is not created yet, it returns the class name of the + * {@link EndpointSelectionStrategy}. + */ + protected String toStringSelector() { + final EndpointSelector endpointSelector = selector.get(); + if (endpointSelector == null) { + // Return selection strategy if selector is not created yet. + return selectionStrategy.getClass().toString(); + } + + return endpointSelector.toString(); + } + private class InitialEndpointsFuture extends EventLoopCheckingFuture> { @Override diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/RoundRobinStrategy.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/RoundRobinStrategy.java index 48813621925..c9b9a86b905 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/RoundRobinStrategy.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/RoundRobinStrategy.java @@ -19,6 +19,8 @@ import java.util.List; import java.util.concurrent.atomic.AtomicInteger; +import com.google.common.base.MoreObjects; + import com.linecorp.armeria.client.ClientRequestContext; import com.linecorp.armeria.client.Endpoint; import com.linecorp.armeria.common.annotation.Nullable; @@ -57,5 +59,12 @@ public Endpoint selectNow(ClientRequestContext ctx) { final int currentSequence = sequence.getAndIncrement(); return endpoints.get(Math.abs(currentSequence % endpoints.size())); } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("endpoints", group().endpoints()) + .toString(); + } } } diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/StickyEndpointSelectionStrategy.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/StickyEndpointSelectionStrategy.java index 18415603dd2..14d48680e1b 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/StickyEndpointSelectionStrategy.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/StickyEndpointSelectionStrategy.java @@ -20,6 +20,7 @@ import java.util.List; import java.util.function.ToLongFunction; +import com.google.common.base.MoreObjects; import com.google.common.hash.Hashing; import com.linecorp.armeria.client.ClientRequestContext; @@ -84,7 +85,6 @@ private static final class StickyEndpointSelector extends AbstractEndpointSelect @Nullable @Override public Endpoint selectNow(ClientRequestContext ctx) { - final List endpoints = group().endpoints(); if (endpoints.isEmpty()) { return null; @@ -94,5 +94,12 @@ public Endpoint selectNow(ClientRequestContext ctx) { final int nearest = Hashing.consistentHash(key, endpoints.size()); return endpoints.get(nearest); } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("endpoints", group().endpoints()) + .toString(); + } } } 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 c318c9124dd..88f124a84b1 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 @@ -22,11 +22,10 @@ import static com.linecorp.armeria.client.endpoint.WeightRampingUpStrategyBuilder.defaultTransition; import static com.linecorp.armeria.internal.client.endpoint.EndpointAttributeKeys.createdAtNanos; import static com.linecorp.armeria.internal.client.endpoint.EndpointAttributeKeys.hasCreatedAtNanos; +import static com.linecorp.armeria.internal.client.endpoint.EndpointToStringUtil.toShortString; import static java.util.Objects.requireNonNull; -import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Deque; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -37,6 +36,9 @@ import java.util.concurrent.TimeUnit; import java.util.function.Supplier; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; @@ -76,6 +78,8 @@ */ final class WeightRampingUpStrategy implements EndpointSelectionStrategy { + private static final Logger logger = LoggerFactory.getLogger(WeightRampingUpStrategy.class); + private static final Ticker defaultTicker = Ticker.systemTicker(); private static final WeightedRandomDistributionEndpointSelector EMPTY_SELECTOR = new WeightedRandomDistributionEndpointSelector(ImmutableList.of()); @@ -130,8 +134,6 @@ final class RampingUpEndpointWeightSelector extends AbstractEndpointSelector { private final List endpointsFinishedRampingUp = new ArrayList<>(); - @VisibleForTesting - final Deque endpointsRampingUp = new ArrayDeque<>(); @VisibleForTesting final Map rampingUpWindowsMap = new HashMap<>(); private Object2LongOpenHashMap endpointCreatedTimestamps = new Object2LongOpenHashMap<>(); @@ -233,7 +235,25 @@ private void buildEndpointSelector() { endpointAndStep.endpoint().withWeight(endpointAndStep.currentWeight())); } } - endpointSelector = new WeightedRandomDistributionEndpointSelector(targetEndpointsBuilder.build()); + final List endpoints = targetEndpointsBuilder.build(); + if (rampingUpWindowsMap.isEmpty()) { + logger.info("Finished ramping up. endpoints: {}", toShortString(endpoints)); + } else { + logger.debug("Ramping up. endpoints: {}", toShortString(endpoints)); + } + + boolean found = false; + for (Endpoint endpoint : endpoints) { + if (endpoint.weight() > 0) { + found = true; + break; + } + } + if (!found) { + logger.warn("No valid endpoint with weight > 0. endpoints: {}", toShortString(endpoints)); + } + + endpointSelector = new WeightedRandomDistributionEndpointSelector(endpoints); } @VisibleForTesting @@ -288,6 +308,15 @@ private void close() { lock.unlock(); } } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("endpointSelector", endpointSelector) + .add("endpointsFinishedRampingUp", endpointsFinishedRampingUp) + .add("rampingUpWindowsMap", rampingUpWindowsMap) + .toString(); + } } private static int numStep(long rampingUpIntervalNanos, Ticker ticker, long createTimestamp) { diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyBuilder.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyBuilder.java index 1712186efbd..c8ef4a3c336 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyBuilder.java @@ -43,9 +43,17 @@ public final class WeightRampingUpStrategyBuilder { static final int DEFAULT_TOTAL_STEPS = 10; static final int DEFAULT_RAMPING_UP_TASK_WINDOW_MILLIS = 500; static final EndpointWeightTransition DEFAULT_LINEAR_TRANSITION = - (endpoint, currentStep, totalSteps) -> - // currentStep is never greater than totalSteps so we can cast long to int. - Ints.saturatedCast((long) endpoint.weight() * currentStep / totalSteps); + (endpoint, currentStep, totalSteps) -> { + // currentStep is never greater than totalSteps so we can cast long to int. + final int currentWeight = + Ints.saturatedCast((long) endpoint.weight() * currentStep / totalSteps); + if (endpoint.weight() > 0 && currentWeight == 0) { + // If the original weight is not 0, + // we should return 1 to make sure the endpoint is selected. + return 1; + } + return currentWeight; + }; static final EndpointWeightTransition defaultTransition = EndpointWeightTransition.linear(); private EndpointWeightTransition transition = defaultTransition; 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 d4def3f082a..4150420a1ae 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 @@ -17,11 +17,16 @@ package com.linecorp.armeria.client.endpoint; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.linecorp.armeria.internal.client.endpoint.EndpointToStringUtil.toShortString; import java.util.Comparator; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.common.collect.Streams; @@ -31,6 +36,8 @@ final class WeightedRoundRobinStrategy implements EndpointSelectionStrategy { + private static final Logger logger = LoggerFactory.getLogger(WeightedRoundRobinStrategy.class); + static final WeightedRoundRobinStrategy INSTANCE = new WeightedRoundRobinStrategy(); private WeightedRoundRobinStrategy() {} @@ -63,6 +70,17 @@ private static final class WeightedRoundRobinSelector extends AbstractEndpointSe @Override protected void updateNewEndpoints(List endpoints) { + boolean found = false; + for (Endpoint endpoint : endpoints) { + if (endpoint.weight() > 0) { + found = true; + break; + } + } + if (!found) { + logger.warn("No valid endpoint with weight > 0. endpoints: {}", toShortString(endpoints)); + } + final EndpointsAndWeights endpointsAndWeights = this.endpointsAndWeights; if (endpointsAndWeights == null || endpointsAndWeights.endpoints != endpoints) { this.endpointsAndWeights = new EndpointsAndWeights(endpoints); @@ -94,6 +112,13 @@ private static final class EndpointsGroupByWeight { } } + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("endpointsAndWeights", endpointsAndWeights) + .toString(); + } + // // In general, assume the weights are w0 < w1 < ... < wM where M = N - 1, N is number of endpoints. // @@ -228,6 +253,16 @@ Endpoint selectEndpoint(int currentSequence) { return endpoints.get(Math.abs(currentSequence % endpoints.size())); } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("endpoints", endpoints) + .add("weighted", weighted) + .add("totalWeight", totalWeight) + .add("accumulatedGroups", accumulatedGroups) + .toString(); + } } } } diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java index 1e480c2060f..637da21fe30 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java @@ -378,7 +378,7 @@ public String toString() { .add("numEndpoints", endpoints.size()) .add("candidates", truncate(delegateEndpoints, 10)) .add("numCandidates", delegateEndpoints.size()) - .add("selectionStrategy", selectionStrategy().getClass()) + .add("selector", toStringSelector()) .add("initialized", whenReady().isDone()) .add("initialSelectionTimeoutMillis", initialSelectionTimeoutMillis) .add("selectionTimeoutMillis", selectionTimeoutMillis) diff --git a/core/src/main/java/com/linecorp/armeria/common/metric/MeterIdPrefixFunction.java b/core/src/main/java/com/linecorp/armeria/common/metric/MeterIdPrefixFunction.java index 48c6f6bb7fe..cf0e331eed3 100644 --- a/core/src/main/java/com/linecorp/armeria/common/metric/MeterIdPrefixFunction.java +++ b/core/src/main/java/com/linecorp/armeria/common/metric/MeterIdPrefixFunction.java @@ -55,6 +55,7 @@ public interface MeterIdPrefixFunction { *
  • Client-side tags:
      *
    • {@code method} - RPC method name or {@link HttpMethod#name()} if RPC method name is not * available
      • + *
    • {@code service} - RPC service name or innermost service class name
      • *
    • {@code httpStatus} - {@link HttpStatus#code()}
      • *
    • * diff --git a/core/src/main/java/com/linecorp/armeria/internal/client/endpoint/EndpointToStringUtil.java b/core/src/main/java/com/linecorp/armeria/internal/client/endpoint/EndpointToStringUtil.java new file mode 100644 index 00000000000..35ac7e92dcd --- /dev/null +++ b/core/src/main/java/com/linecorp/armeria/internal/client/endpoint/EndpointToStringUtil.java @@ -0,0 +1,61 @@ +/* + * 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.internal.client.endpoint; + +import java.util.List; + +import com.linecorp.armeria.client.Endpoint; +import com.linecorp.armeria.internal.common.util.TemporaryThreadLocals; + +public final class EndpointToStringUtil { + + public static String toShortString(List endpoints) { + try (TemporaryThreadLocals acquired = TemporaryThreadLocals.acquire()) { + final StringBuilder builder = acquired.stringBuilder(); + builder.append('['); + for (int i = 0; i < endpoints.size(); i++) { + if (i > 0) { + builder.append(", "); + } + final Endpoint endpoint = endpoints.get(i); + toShortString(builder, endpoint); + } + builder.append(']'); + return builder.toString(); + } + } + + public static String toShortString(Endpoint endpoint) { + try (TemporaryThreadLocals acquired = TemporaryThreadLocals.acquire()) { + final StringBuilder builder = acquired.stringBuilder(); + toShortString(builder, endpoint); + return builder.toString(); + } + } + + private static void toShortString(StringBuilder builder, Endpoint endpoint) { + builder.append(endpoint.host()); + if (endpoint.hasIpAddr() && !endpoint.isIpAddrOnly()) { + builder.append('/').append(endpoint.ipAddr()); + } + if (endpoint.hasPort()) { + builder.append(':').append(endpoint.port()); + } + builder.append(" (weight: ").append(endpoint.weight()).append(')'); + } + + private EndpointToStringUtil() {} +} diff --git a/core/src/main/java/com/linecorp/armeria/internal/client/endpoint/WeightedRandomDistributionSelector.java b/core/src/main/java/com/linecorp/armeria/internal/client/endpoint/WeightedRandomDistributionSelector.java index 0c0d40ed807..a36f8a00f14 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/client/endpoint/WeightedRandomDistributionSelector.java +++ b/core/src/main/java/com/linecorp/armeria/internal/client/endpoint/WeightedRandomDistributionSelector.java @@ -22,6 +22,7 @@ import java.util.concurrent.locks.ReentrantLock; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.concurrent.GuardedBy; @@ -107,6 +108,17 @@ public T select() { throw new Error("Should never reach here"); } + @SuppressWarnings("GuardedBy") + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("allEntries", allEntries) + .add("currentEntries", currentEntries) + .add("total", total) + .add("remaining", remaining) + .toString(); + } + public abstract static class AbstractEntry { private int counter; diff --git a/core/src/test/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyTest.java b/core/src/test/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyTest.java index 54ede19bbe3..6542f67bb96 100644 --- a/core/src/test/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/endpoint/WeightRampingUpStrategyTest.java @@ -110,7 +110,6 @@ void rampingUpIsDoneAfterNumberOfSteps() { scheduledJobs.poll().run(); // Ramping up is done because the step reached the numberOfSteps. - assertThat(selector.endpointsRampingUp).isEmpty(); endpointsFromEntry = endpointsFromSelectorEntry(selector); assertThat(endpointsFromEntry).usingElementComparator(EndpointComparator.INSTANCE) .containsExactlyInAnyOrder( From 4ba153143fb386b5dfbcc14e8200fdd26ca5a757 Mon Sep 17 00:00:00 2001 From: minux Date: Tue, 3 Dec 2024 21:05:13 +0900 Subject: [PATCH 23/27] Add the release note for 1.31.2 (#6013) --- site/src/pages/release-notes/1.31.2.mdx | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 site/src/pages/release-notes/1.31.2.mdx diff --git a/site/src/pages/release-notes/1.31.2.mdx b/site/src/pages/release-notes/1.31.2.mdx new file mode 100644 index 00000000000..68daf15d4ea --- /dev/null +++ b/site/src/pages/release-notes/1.31.2.mdx @@ -0,0 +1,18 @@ +--- +date: 2024-12-03 +--- + +## 🛠️ Bug fixes + +- Fixed a bug where stale endpoints remained when a Kubernetes service was updated. #6012 +- Fix a bug where weights could unintentionally be set to 0 in ramping-up strategies. #6014 + +## 🙇 Thank you + + From e649318cf16248784936d536e91417e32338b323 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 5 Dec 2024 10:07:43 +0000 Subject: [PATCH 24/27] Update public suffix list (#6020) Automated changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action Co-authored-by: Meri Kim --- .../src/main/resources/com/linecorp/armeria/public_suffixes.txt | 2 +- 1 file changed, 1 insertion(+), 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 f87101724ae..d9451e3585d 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -7,6 +7,7 @@ !city.yokohama.jp !www.ck *.001.test.code-builder-stg.platform.salesforce.com +*.0e.vc *.0emm.com *.advisor.ws *.af-south-1.airflow.amazonaws.com @@ -169,7 +170,6 @@ 0.bg 001www.com 0am.jp -0e.vc 0g0.jp 0j0.jp 0t0.jp From fa76e99fa6132545df3a8d05eeb81c5681ec8953 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Fri, 6 Dec 2024 11:15:50 +0900 Subject: [PATCH 25/27] Use `macos-latest` for CI builds (#6022) Motivation: macos-12 is no longer supported in GitHub Actions. - https://github.com/line/armeria/actions/runs/12177357989?pr=5941 - https://github.com/actions/runner-images/issues/10721 Modifications: - Use macos-latest instead. Result: Revive macOS CI --- .github/workflows/actions_build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/actions_build.yml b/.github/workflows/actions_build.yml index ddd7166a80c..14602c7abd7 100644 --- a/.github/workflows/actions_build.yml +++ b/.github/workflows/actions_build.yml @@ -32,7 +32,7 @@ jobs: strategy: fail-fast: false matrix: - on: [ ubicloud-standard-8, macos-12, windows-latest ] + on: [ ubicloud-standard-8, macos-latest, windows-latest ] java: [ 21 ] include: - java: 8 From e3248a118aa2583851112290d61ed8e3039553a4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 10 Dec 2024 10:07:40 +0000 Subject: [PATCH 26/27] Update public suffix list (#6027) 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 d9451e3585d..a8a9ec43377 100644 --- a/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt +++ b/core/src/main/resources/com/linecorp/armeria/public_suffixes.txt @@ -5827,6 +5827,7 @@ netbank netflix netfy.app netgamers.jp +netlib.re netlify.app network neustar @@ -6221,6 +6222,7 @@ omotego.fukushima.jp omura.nagasaki.jp omuta.fukuoka.jp on-aptible.com +on-fleek.app on-the-web.tv on-web.fr on.biz.ng @@ -8290,7 +8292,6 @@ terni.it ternopil.ua teshikaga.hokkaido.jp test-iserv.de -test.ru test.tj tests.cx teva From 244e5cb661c8990a771db1696aa7da558fd7be4a Mon Sep 17 00:00:00 2001 From: jrhee17 Date: Wed, 11 Dec 2024 11:16:54 +0900 Subject: [PATCH 27/27] `ResponseTimeoutMode.FROM_START` works correctly with `RetryingClient` (#6025) Motivation: A bug was reported that `ResponseTimeoutMode.FROM_START` does not work correctly when used with a `RetryingClient`. The cause was because how the `responseTimeout` is calculated for `RetryingClient`. `RetryingClient` bounds `responseTimeout` by computing the `responseTimeout` on each iteration from its internal `State`. https://github.com/line/armeria/blob/fa76e99fa6132545df3a8d05eeb81c5681ec8953/core/src/main/java/com/linecorp/armeria/client/retry/AbstractRetryingClient.java#L188 If the `CancellationScheduler` has not been started yet, the set timeout is returned as-is via `CancellationScheduler#timeoutNanos` and is set at for the derived ctx. https://github.com/line/armeria/blob/fa76e99fa6132545df3a8d05eeb81c5681ec8953/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java#L543-L544 However, `CancellationScheduler#timeoutNanos` defines its contract as returning the `timeoutNanos` if not started, and returning `timeoutNanos` since the `startTime` if already started. https://github.com/line/armeria/blob/fa76e99fa6132545df3a8d05eeb81c5681ec8953/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java#L104-L108 Hence, `CancellationScheduler#setTimeoutNanos` tries to set the time remaining, but `CancellationScheduler#timeoutNanos` will return the timeout since `CancellationScheduler#start` is called. Since the semantics of `CancellationScheduler#timeoutNanos` has value in retaining the originally set value, I propose that a new `CancellationScheduler#remainingTimeoutNanos` is introduced which returns the remaining timeout if a scheduler has been started. Modifications: - Introduced `CancellationScheduler#remainingTimeoutNanos` which returns the remaining `responseTimeout` in nanos. - Replaced `ClientRequestContext#responseTimeoutMillis` with `ClientRequestContextExtension#remainingTimeoutNanos` in `ArmeriaClientCall` and `DefaultClientRequestContext` - Removed unneeded usages of `ClientRequestContext#responseTimeoutMillis` in `HttpResponseWrapper` Result: - `ResponseTimeoutMode.FROM_START` correctly bounds requests that go through `RetryingClient` --- .../client/AbstractHttpResponseDecoder.java | 3 +- .../armeria/client/HttpResponseWrapper.java | 9 +- .../WebSocketHttp1ClientChannelHandler.java | 3 +- .../client/WebSocketHttp1ResponseWrapper.java | 5 +- .../client/ClientRequestContextExtension.java | 2 + .../client/DefaultClientRequestContext.java | 8 +- .../common/CancellationScheduler.java | 6 ++ .../common/DefaultCancellationScheduler.java | 21 +++- .../common/NoopCancellationScheduler.java | 5 + .../retry/ResponseTimeoutFromStartTest.java | 97 +++++++++++++++++++ .../client/grpc/ArmeriaClientCall.java | 2 +- 11 files changed, 144 insertions(+), 17 deletions(-) create mode 100644 core/src/test/java/com/linecorp/armeria/client/retry/ResponseTimeoutFromStartTest.java diff --git a/core/src/main/java/com/linecorp/armeria/client/AbstractHttpResponseDecoder.java b/core/src/main/java/com/linecorp/armeria/client/AbstractHttpResponseDecoder.java index e1ba730eb38..c477c008389 100644 --- a/core/src/main/java/com/linecorp/armeria/client/AbstractHttpResponseDecoder.java +++ b/core/src/main/java/com/linecorp/armeria/client/AbstractHttpResponseDecoder.java @@ -62,8 +62,7 @@ public HttpResponseWrapper addResponse(@Nullable AbstractHttpRequestHandler requ int id, DecodedHttpResponse res, ClientRequestContext ctx, EventLoop eventLoop) { final HttpResponseWrapper newRes = - new HttpResponseWrapper(requestHandler, res, eventLoop, ctx, - ctx.responseTimeoutMillis(), ctx.maxResponseLength()); + new HttpResponseWrapper(requestHandler, res, eventLoop, ctx, ctx.maxResponseLength()); final HttpResponseWrapper oldRes = responses.put(id, newRes); keepAliveHandler().increaseNumRequests(); diff --git a/core/src/main/java/com/linecorp/armeria/client/HttpResponseWrapper.java b/core/src/main/java/com/linecorp/armeria/client/HttpResponseWrapper.java index 27aafaabd9c..dfd3a91fb6a 100644 --- a/core/src/main/java/com/linecorp/armeria/client/HttpResponseWrapper.java +++ b/core/src/main/java/com/linecorp/armeria/client/HttpResponseWrapper.java @@ -58,7 +58,6 @@ class HttpResponseWrapper implements StreamWriter { private final EventLoop eventLoop; private final ClientRequestContext ctx; private final long maxContentLength; - private final long responseTimeoutMillis; private boolean responseStarted; private long contentLengthHeaderValue = -1; @@ -66,15 +65,14 @@ class HttpResponseWrapper implements StreamWriter { private boolean done; private boolean closed; - HttpResponseWrapper(@Nullable AbstractHttpRequestHandler requestHandler, - DecodedHttpResponse delegate, EventLoop eventLoop, ClientRequestContext ctx, - long responseTimeoutMillis, long maxContentLength) { + HttpResponseWrapper(@Nullable AbstractHttpRequestHandler requestHandler, DecodedHttpResponse delegate, + EventLoop eventLoop, ClientRequestContext ctx, long maxContentLength) { + this.requestHandler = requestHandler; this.delegate = delegate; this.eventLoop = eventLoop; this.ctx = ctx; this.maxContentLength = maxContentLength; - this.responseTimeoutMillis = responseTimeoutMillis; } void handle100Continue(ResponseHeaders responseHeaders) { @@ -327,7 +325,6 @@ public String toString() { .add("eventLoop", eventLoop) .add("responseStarted", responseStarted) .add("maxContentLength", maxContentLength) - .add("responseTimeoutMillis", responseTimeoutMillis) .add("contentLengthHeaderValue", contentLengthHeaderValue) .add("delegate", delegate) .toString(); diff --git a/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ClientChannelHandler.java b/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ClientChannelHandler.java index f80cbe0c4c7..cf6d0171792 100644 --- a/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ClientChannelHandler.java +++ b/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ClientChannelHandler.java @@ -100,8 +100,7 @@ public HttpResponseWrapper addResponse(@Nullable AbstractHttpRequestHandler requ int id, DecodedHttpResponse decodedHttpResponse, ClientRequestContext ctx, EventLoop eventLoop) { assert res == null; - res = new WebSocketHttp1ResponseWrapper(decodedHttpResponse, eventLoop, ctx, - ctx.responseTimeoutMillis(), ctx.maxResponseLength()); + res = new WebSocketHttp1ResponseWrapper(decodedHttpResponse, eventLoop, ctx, ctx.maxResponseLength()); return res; } diff --git a/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ResponseWrapper.java b/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ResponseWrapper.java index 6d920f7116f..47abd673080 100644 --- a/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ResponseWrapper.java +++ b/core/src/main/java/com/linecorp/armeria/client/WebSocketHttp1ResponseWrapper.java @@ -26,9 +26,8 @@ final class WebSocketHttp1ResponseWrapper extends HttpResponseWrapper { WebSocketHttp1ResponseWrapper(DecodedHttpResponse delegate, - EventLoop eventLoop, ClientRequestContext ctx, - long responseTimeoutMillis, long maxContentLength) { - super(null, delegate, eventLoop, ctx, responseTimeoutMillis, maxContentLength); + EventLoop eventLoop, ClientRequestContext ctx, long maxContentLength) { + super(null, delegate, eventLoop, ctx, maxContentLength); WebSocketClientUtil.setClosingResponseTask(ctx, cause -> { super.close(cause, false); }); diff --git a/core/src/main/java/com/linecorp/armeria/internal/client/ClientRequestContextExtension.java b/core/src/main/java/com/linecorp/armeria/internal/client/ClientRequestContextExtension.java index a818d4c8994..26a9081031e 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/client/ClientRequestContextExtension.java +++ b/core/src/main/java/com/linecorp/armeria/internal/client/ClientRequestContextExtension.java @@ -73,4 +73,6 @@ public interface ClientRequestContextExtension extends ClientRequestContext, Req * with default values on every request. */ HttpHeaders internalRequestHeaders(); + + long remainingTimeoutNanos(); } 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..4ad2e2bdeee 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 @@ -540,8 +540,7 @@ private DefaultClientRequestContext(DefaultClientRequestContext ctx, log.startRequest(); // Cancel the original timeout and create a new scheduler for the derived context. ctx.responseCancellationScheduler.cancelScheduled(); - responseCancellationScheduler = - CancellationScheduler.ofClient(TimeUnit.MILLISECONDS.toNanos(ctx.responseTimeoutMillis())); + responseCancellationScheduler = CancellationScheduler.ofClient(ctx.remainingTimeoutNanos()); writeTimeoutMillis = ctx.writeTimeoutMillis(); maxResponseLength = ctx.maxResponseLength(); @@ -898,6 +897,11 @@ public HttpHeaders internalRequestHeaders() { return internalRequestHeaders; } + @Override + public long remainingTimeoutNanos() { + return responseCancellationScheduler().remainingTimeoutNanos(); + } + @Override public void setAdditionalRequestHeader(CharSequence name, Object value) { requireNonNull(name, "name"); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java b/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java index a5b91af0950..7f5ff402ee6 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/CancellationScheduler.java @@ -108,6 +108,12 @@ default void finishNow() { */ long timeoutNanos(); + /** + * Before the scheduler has started, the configured timeout will be returned regardless of the + * {@link TimeoutMode}. If the scheduler has already started, the remaining time will be returned. + */ + long remainingTimeoutNanos(); + long startTimeNanos(); CompletableFuture whenCancelling(); diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java b/core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java index 698aa7e523f..f0e7bddd380 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/DefaultCancellationScheduler.java @@ -112,12 +112,14 @@ public void start() { if (state != State.INIT) { return; } - state = State.SCHEDULED; startTimeNanos = ticker.read(); if (timeoutMode == TimeoutMode.SET_FROM_NOW) { final long elapsedTimeNanos = startTimeNanos - setFromNowStartNanos; timeoutNanos = Long.max(LongMath.saturatedSubtract(timeoutNanos, elapsedTimeNanos), 0); } + + // set the state after all timeout related fields are updated + state = State.SCHEDULED; if (timeoutNanos != Long.MAX_VALUE) { scheduledFuture = eventLoop().schedule(() -> invokeTask(null), timeoutNanos, NANOSECONDS); } @@ -292,6 +294,23 @@ public long timeoutNanos() { return timeoutNanos == Long.MAX_VALUE ? 0 : timeoutNanos; } + @Override + public long remainingTimeoutNanos() { + lock.lock(); + try { + if (timeoutNanos == Long.MAX_VALUE) { + return 0; + } + if (!isStarted()) { + return timeoutNanos; + } + final long elapsed = ticker.read() - startTimeNanos; + return Math.max(1, LongMath.saturatedSubtract(timeoutNanos, elapsed)); + } finally { + lock.unlock(); + } + } + @Override public long startTimeNanos() { return startTimeNanos; diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/NoopCancellationScheduler.java b/core/src/main/java/com/linecorp/armeria/internal/common/NoopCancellationScheduler.java index c6f6ac71b83..4bd6e94ffc9 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/NoopCancellationScheduler.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/NoopCancellationScheduler.java @@ -90,6 +90,11 @@ public long timeoutNanos() { return 0; } + @Override + public long remainingTimeoutNanos() { + return 0; + } + @Override public long startTimeNanos() { return 0; diff --git a/core/src/test/java/com/linecorp/armeria/client/retry/ResponseTimeoutFromStartTest.java b/core/src/test/java/com/linecorp/armeria/client/retry/ResponseTimeoutFromStartTest.java new file mode 100644 index 00000000000..077f2f88106 --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/client/retry/ResponseTimeoutFromStartTest.java @@ -0,0 +1,97 @@ +/* + * 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.client.retry; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; + +import java.time.Duration; +import java.util.concurrent.CompletionException; +import java.util.concurrent.TimeUnit; + +import org.assertj.core.data.Percentage; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.linecorp.armeria.client.ResponseTimeoutException; +import com.linecorp.armeria.client.ResponseTimeoutMode; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.QueryParams; +import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; + +class ResponseTimeoutFromStartTest { + + private static final Logger logger = LoggerFactory.getLogger(ResponseTimeoutFromStartTest.class); + + @RegisterExtension + static ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) throws Exception { + sb.service("/", (ctx, req) -> { + final String delayMillisStr = ctx.queryParam("delayMillis"); + assertThat(delayMillisStr).isNotNull(); + final int delayMillis = Integer.parseInt(delayMillisStr); + return HttpResponse.delayed(HttpResponse.of(500), Duration.ofMillis(delayMillis)); + }); + } + }; + + @ParameterizedTest + @CsvSource({ + "0,2500,2000", + "0,1750,2000", + "5000,1500,2000", + }) + void originalResponseTimeoutRespected(long backoffMillis, long attemptMillis, long delayMillis) { + final long timeoutSeconds = 3; + final WebClient webClient = + WebClient.builder(server.httpUri()) + .responseTimeout(Duration.ofSeconds(timeoutSeconds)) + .responseTimeoutMode(ResponseTimeoutMode.FROM_START) + .decorator( + RetryingClient.builder(RetryRule.builder() + .onException() + .onServerErrorStatus() + .thenBackoff(Backoff.fixed(backoffMillis))) + .responseTimeoutForEachAttempt(Duration.ofMillis(attemptMillis)) + .maxTotalAttempts(Integer.MAX_VALUE) + .newDecorator()) + .build(); + + final long prev = System.nanoTime(); + final Throwable throwable = catchThrowable( + () -> webClient.get("/", QueryParams.of("delayMillis", delayMillis)).aggregate().join()); + assertThat(throwable) + .isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(ResponseTimeoutException.class); + logger.debug("elapsed time is: {}ms", TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - prev)); + + if (backoffMillis > 0) { + assertThat(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - prev)) + .isLessThan(TimeUnit.SECONDS.toMillis(timeoutSeconds)); + } else { + + assertThat(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - prev)) + .isCloseTo(TimeUnit.SECONDS.toMillis(timeoutSeconds), Percentage.withPercentage(10)); + } + } +} 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 e996aa012b8..d00c4d572cc 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 @@ -241,7 +241,7 @@ public void start(Listener responseListener, Metadata metadata) { ctx.setResponseTimeout(TimeoutMode.SET_FROM_NOW, Duration.ofNanos(remainingNanos)); } } else { - remainingNanos = MILLISECONDS.toNanos(ctx.responseTimeoutMillis()); + remainingNanos = ctx.remainingTimeoutNanos(); } // Must come after handling deadline.