From 5b6a9fde32ab697fc0bd741e20acab40286c0dc9 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 12 Apr 2024 16:24:23 +0900 Subject: [PATCH 01/24] refactor: remove the Force shutdown mode --- .../armeria/server/AbstractHttpResponseHandler.java | 7 ------- 1 file changed, 7 deletions(-) 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 43a229e3330..12d5b4d0f0e 100644 --- a/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseHandler.java @@ -56,7 +56,6 @@ abstract class AbstractHttpResponseHandler { private final CompletableFuture completionFuture; private boolean isComplete; - private boolean needsDisconnection; AbstractHttpResponseHandler(ChannelHandlerContext ctx, ServerHttpObjectEncoder responseEncoder, @@ -77,7 +76,6 @@ boolean isDone() { } void disconnectWhenFinished() { - needsDisconnection = true; responseEncoder.keepAliveHandler().disconnectWhenFinished(); } @@ -92,11 +90,6 @@ final boolean tryComplete(@Nullable Throwable cause) { completionFuture.completeExceptionally(cause); } - // Force shutdown mode: If a user explicitly sets `Connection: close` in the response headers, it is - // assumed that the connection should be closed after sending the response. - if (needsDisconnection) { - ctx.channel().close(); - } return true; } From f01ca8ab80e658c9055208ee60b439f81df14c0e Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 12 Apr 2024 16:51:20 +0900 Subject: [PATCH 02/24] feat: create a schedule for waiting and closing in HTTP1 --- .../java/com/linecorp/armeria/server/HttpServerHandler.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 9bab1110bd9..9358c50909a 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -780,7 +780,9 @@ private void handleRequestOrResponseComplete() { // Stop receiving new requests. handledLastRequest = true; if (unfinishedRequests.isEmpty()) { - ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); + ctx.executor().schedule(() -> + ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE), + 1000, TimeUnit.MILLISECONDS); } } } From 824bb23827ebaf629f578a813d14a28ebcc8106e Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Wed, 24 Apr 2024 08:37:41 +0900 Subject: [PATCH 03/24] feat: define a defaultHttp1ConnectionCloseDelayMillis Flag --- .../armeria/common/DefaultFlagsProvider.java | 6 ++++++ .../com/linecorp/armeria/common/Flags.java | 19 +++++++++++++++++++ .../armeria/common/FlagsProvider.java | 16 ++++++++++++++++ .../common/SystemPropertyFlagsProvider.java | 5 +++++ 4 files changed, 46 insertions(+) diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java index be234e7e625..067ffb25eec 100644 --- a/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java @@ -93,6 +93,7 @@ final class DefaultFlagsProvider implements FlagsProvider { static final String FILE_SERVICE_CACHE_SPEC = "maximumSize=1024"; static final String DNS_CACHE_SPEC = "maximumSize=4096"; static final long DEFAULT_UNHANDLED_EXCEPTIONS_REPORT_INTERVAL_MILLIS = 10000; + static final long DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS = 3000; private DefaultFlagsProvider() {} @@ -479,4 +480,9 @@ public Long defaultUnhandledExceptionsReportIntervalMillis() { public DistributionStatisticConfig distributionStatisticConfig() { return DistributionStatisticConfigUtil.DEFAULT_DIST_STAT_CFG; } + + @Override + public Long defaultHttp1ConnectionCloseDelayMillis() { + return DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS; + } } diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index 58e7041189b..43f54d8f6f3 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -412,6 +412,10 @@ private static boolean validateTransportType(TransportType transportType, String private static final DistributionStatisticConfig DISTRIBUTION_STATISTIC_CONFIG = getValue(FlagsProvider::distributionStatisticConfig, "distributionStatisticConfig"); + private static final long DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS = + getValue(FlagsProvider::defaultHttp1ConnectionCloseDelayMillis, + "defaultHttp1ConnectionCloseDelayMillis", value -> value >= 0); + /** * Returns the specification of the {@link Sampler} that determines whether to retain the stack * trace of the exceptions that are thrown frequently by Armeria. A sampled exception will have the stack @@ -1539,6 +1543,21 @@ public static DistributionStatisticConfig distributionStatisticConfig() { return DISTRIBUTION_STATISTIC_CONFIG; } + /** + * Returns the default time in milliseconds to wait before closing an HTTP1 connection when a server needs + * to close the connection. This allows to avoid a server socket from remaining in the TIME_WAIT state + * instead of CLOSED when a connection is closed. + * + *

The default value of this flag is + * {@value DefaultFlagsProvider#DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS}. Specify the + * {@code -Dcom.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis=} JVM option to + * override the default value.

+ */ + @UnstableApi + public static long defaultHttp1ConnectionCloseDelayMillis() { + return DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS; + } + @Nullable private static String nullableCaffeineSpec(Function method, String flagName) { return caffeineSpec(method, flagName, true); diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index e2b606fc1c2..496d66dde3a 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -1159,4 +1159,20 @@ default Long defaultUnhandledExceptionsReportIntervalMillis() { default DistributionStatisticConfig distributionStatisticConfig() { return null; } + + /** + * Returns the default time in milliseconds to wait before closing an HTTP1 connection when a server needs + * to close the connection. This allows to avoid a server socket from remaining in the TIME_WAIT state + * instead of CLOSED when a connection is closed. + * + *

The default value of this flag is + * {@value DefaultFlagsProvider#DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS}. Specify the + * {@code -Dcom.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis=} JVM option to + * override the default value.

+ */ + @Nullable + @UnstableApi + default Long defaultHttp1ConnectionCloseDelayMillis() { + return null; + } } diff --git a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java index d1b3411fbd1..f34057eee6e 100644 --- a/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/SystemPropertyFlagsProvider.java @@ -492,6 +492,11 @@ public Long defaultUnhandledExceptionsReportIntervalMillis() { return getLong("defaultUnhandledExceptionsReportIntervalMillis"); } + @Override + public Long defaultHttp1ConnectionCloseDelayMillis() { + return getLong("defaultHttp1ConnectionCloseDelayMillis"); + } + @Nullable private static Long getLong(String name) { return getAndParse(name, Long::parseLong); From 1d764aadcf84f7b938e190bd29d2ba347e20f28e Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Wed, 24 Apr 2024 08:40:02 +0900 Subject: [PATCH 04/24] refactor: replace hardcoded defaultHttp1ConnectionCloseDelayMillis with the flag --- .../java/com/linecorp/armeria/server/HttpServerHandler.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 9358c50909a..b7e5e021217 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -35,6 +35,7 @@ import javax.net.ssl.SSLSession; +import com.linecorp.armeria.common.Flags; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -781,8 +782,8 @@ private void handleRequestOrResponseComplete() { handledLastRequest = true; if (unfinishedRequests.isEmpty()) { ctx.executor().schedule(() -> - ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE), - 1000, TimeUnit.MILLISECONDS); + ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE), + Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); } } } From 5820fe9a12b459caacc43a069ee50fa57eefc3cb Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Wed, 24 Apr 2024 08:53:18 +0900 Subject: [PATCH 05/24] fix: to schedule the close connection task to run in the channel's event loop --- .../java/com/linecorp/armeria/server/HttpServerHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b7e5e021217..82631365467 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -781,7 +781,7 @@ private void handleRequestOrResponseComplete() { // Stop receiving new requests. handledLastRequest = true; if (unfinishedRequests.isEmpty()) { - ctx.executor().schedule(() -> + ctx.channel().eventLoop().schedule(() -> ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE), Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); } From 4a7f0efdb37f05fba6faf8b91d970984f421c3ee Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Wed, 24 Apr 2024 09:20:38 +0900 Subject: [PATCH 06/24] refactor: fix import order --- .../java/com/linecorp/armeria/server/HttpServerHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 82631365467..dbb6481f4fd 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -35,12 +35,12 @@ import javax.net.ssl.SSLSession; -import com.linecorp.armeria.common.Flags; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import com.linecorp.armeria.common.AggregationOptions; import com.linecorp.armeria.common.ClosedSessionException; +import com.linecorp.armeria.common.Flags; import com.linecorp.armeria.common.HttpData; import com.linecorp.armeria.common.HttpHeaderNames; import com.linecorp.armeria.common.HttpMethod; From 90b5509da1094ee5314ce4de9a2c2cf555951720 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Wed, 24 Apr 2024 18:12:42 +0900 Subject: [PATCH 07/24] test: set defaultHttp1ConnectionCloseDelayMillis to 0 on HttpServerPathTest --- .../java/com/linecorp/armeria/server/HttpServerPathTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java b/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java index 922a8763892..9557480d446 100644 --- a/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java @@ -26,6 +26,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import org.junitpioneer.jupiter.SetSystemProperty; import com.google.common.io.ByteStreams; @@ -36,6 +37,8 @@ import io.netty.util.NetUtil; +// Allow the server to close the socket immediately, without waiting for the client to close +@SetSystemProperty(key = "com.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis", value = "0") class HttpServerPathTest { @RegisterExtension From ae86f290e440450deb5ea45499832ff15a3a1348 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Wed, 24 Apr 2024 18:14:24 +0900 Subject: [PATCH 08/24] feat: check the active status of a channel before running a close task --- .../com/linecorp/armeria/server/HttpServerHandler.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 dbb6481f4fd..c4278b1c640 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -781,9 +781,11 @@ private void handleRequestOrResponseComplete() { // Stop receiving new requests. handledLastRequest = true; if (unfinishedRequests.isEmpty()) { - ctx.channel().eventLoop().schedule(() -> - ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE), - Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); + ctx.channel().eventLoop().schedule(() -> { + if (ctx.channel().isActive()) { + ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); + } + }, Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); } } } From e74d1825fc27147b1e11a70baf0be732ec96ce01 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Thu, 25 Apr 2024 19:20:19 +0900 Subject: [PATCH 09/24] test: read only a first line in HttpServerPathTest to does not wait for the server to close the connection --- .../linecorp/armeria/server/HttpServerPathTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java b/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java index 9557480d446..f685f1d020f 100644 --- a/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/HttpServerPathTest.java @@ -18,6 +18,8 @@ import static org.assertj.core.api.Assertions.assertThat; +import java.io.BufferedReader; +import java.io.InputStreamReader; import java.net.Socket; import java.nio.charset.StandardCharsets; import java.util.LinkedHashMap; @@ -26,9 +28,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; -import org.junitpioneer.jupiter.SetSystemProperty; - -import com.google.common.io.ByteStreams; import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.HttpStatus; @@ -37,8 +36,6 @@ import io.netty.util.NetUtil; -// Allow the server to close the socket immediately, without waiting for the client to close -@SetSystemProperty(key = "com.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis", value = "0") class HttpServerPathTest { @RegisterExtension @@ -124,7 +121,11 @@ private static void urlPathAssertion(HttpStatus expected, String path) throws Ex try (Socket s = new Socket(NetUtil.LOCALHOST, server.httpPort())) { s.setSoTimeout(10000); s.getOutputStream().write(requestString.getBytes(StandardCharsets.US_ASCII)); - assertThat(new String(ByteStreams.toByteArray(s.getInputStream()), StandardCharsets.US_ASCII)) + final BufferedReader in = new BufferedReader( + new InputStreamReader(s.getInputStream(), StandardCharsets.US_ASCII)); + // only reads a first line because it only needs to check the expected status + // and does not wait for the server to close the connection + assertThat(in.readLine()) .as(path) .startsWith("HTTP/1.1 " + expected); } From 4ec928d58d07748e527606d737c212cdfe20c7cb Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 26 Apr 2024 10:31:01 +0900 Subject: [PATCH 10/24] test: implement test cases for Http1ServerDelayedCloseConnectionTest --- ...Http1ServerDelayedCloseConnectionTest.java | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java new file mode 100644 index 00000000000..b87c0baab6c --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -0,0 +1,150 @@ +/* + * Copyright 2022 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.Flags; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpResponse; + +import com.linecorp.armeria.testing.junit5.server.ServerExtension; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.PrintWriter; +import java.net.BindException; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.Socket; +import java.time.Duration; +import java.util.Random; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class Http1ServerDelayedCloseConnectionTest { + + @RegisterExtension + static ServerExtension server = new ServerExtension() { + @Override + protected void configure(ServerBuilder sb) { + sb.idleTimeoutMillis(0); + sb.http(0); + sb.https(0); + sb.tlsSelfSigned(); + sb.service("/close", (ctx, req) -> { + return HttpResponse.builder() + .ok() + .content("OK\n") + .header(HttpHeaderNames.CONNECTION, "close") + .build(); + }); + } + }; + + + @Test + void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() throws IOException { + Random random = new Random(); + short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); + try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { + socket.setReuseAddress(true); + socket.setSoTimeout(100000); + final PrintWriter writer = new PrintWriter(socket.getOutputStream()); + writer.print("GET /close" + " HTTP/1.1\r\n"); + writer.print("\r\n"); + writer.flush(); + + final BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + assertThat(in.readLine()).isEqualTo("HTTP/1.1 200 OK"); + + String line; + boolean hasConnectionClose = false; + while ((line = in.readLine()) != null) { + if ("connection: close".equalsIgnoreCase(line)) { + hasConnectionClose = true; + } + if (line.isEmpty() || line.contains(":")) { + continue; + } + if (line.startsWith("OK")) { + break; + } + } + long readStartTimestamp = System.nanoTime(); + int readResult = in.read(); + long readDurationMillis = Duration.ofNanos(System.nanoTime() - readStartTimestamp).toMillis(); + + assertThat(hasConnectionClose).isTrue(); + assertThat(readResult).isEqualTo(-1); + + long defaultHttp1ConnectionCloseDelayMillis = Flags.defaultHttp1ConnectionCloseDelayMillis(); + assertThat(readDurationMillis).isBetween( + defaultHttp1ConnectionCloseDelayMillis - 100, + defaultHttp1ConnectionCloseDelayMillis + 1000 + ); + + socket.close(); + Socket reuseSock = new Socket(); + reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort)); + reuseSock.close(); + } + } + + @Test + void shouldWaitForDisconnectByClientSideFirst() throws IOException { + Random random = new Random(); + short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); + try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { + socket.setReuseAddress(true); + socket.setSoTimeout(1000); + final PrintWriter writer = new PrintWriter(socket.getOutputStream()); + writer.print("GET /close" + " HTTP/1.1\r\n"); + writer.print("\r\n"); + writer.flush(); + + final BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); + assertThat(in.readLine()).isEqualTo("HTTP/1.1 200 OK"); + + String line; + boolean hasConnectionClose = false; + while ((line = in.readLine()) != null) { + if ("connection: close".equalsIgnoreCase(line)) { + hasConnectionClose = true; + } + if (line.isEmpty() || line.contains(":")) { + continue; + } + if (line.startsWith("OK")) { + break; + } + } + assertThat(hasConnectionClose).isTrue(); + assertThat(server.server().numConnections()).isEqualTo(1); + + socket.close(); + Socket reuseSock = new Socket(); + assertThatThrownBy(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) + .isInstanceOf(BindException.class) + .hasMessageContaining("Address already in use"); + reuseSock.close(); + assertThat(server.server().numConnections()).isZero(); + } + } +} From abbfc56d298ecc151c2c969f931c9db41c7975ef Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 26 Apr 2024 10:44:37 +0900 Subject: [PATCH 11/24] test: fix lint errors --- ...Http1ServerDelayedCloseConnectionTest.java | 41 +++++++++---------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index b87c0baab6c..73fa6f8b6db 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2022 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 @@ -16,13 +16,8 @@ package com.linecorp.armeria.server; -import com.linecorp.armeria.common.Flags; -import com.linecorp.armeria.common.HttpHeaderNames; -import com.linecorp.armeria.common.HttpResponse; - -import com.linecorp.armeria.testing.junit5.server.ServerExtension; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.RegisterExtension; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.io.BufferedReader; import java.io.IOException; @@ -35,8 +30,13 @@ import java.time.Duration; import java.util.Random; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import com.linecorp.armeria.common.Flags; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.testing.junit5.server.ServerExtension; class Http1ServerDelayedCloseConnectionTest { @@ -58,11 +58,10 @@ protected void configure(ServerBuilder sb) { } }; - @Test void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() throws IOException { - Random random = new Random(); - short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); + final Random random = new Random(); + final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { socket.setReuseAddress(true); socket.setSoTimeout(100000); @@ -87,21 +86,21 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr break; } } - long readStartTimestamp = System.nanoTime(); - int readResult = in.read(); - long readDurationMillis = Duration.ofNanos(System.nanoTime() - readStartTimestamp).toMillis(); + final long readStartTimestamp = System.nanoTime(); + final int readResult = in.read(); + final long readDurationMillis = Duration.ofNanos(System.nanoTime() - readStartTimestamp).toMillis(); assertThat(hasConnectionClose).isTrue(); assertThat(readResult).isEqualTo(-1); - long defaultHttp1ConnectionCloseDelayMillis = Flags.defaultHttp1ConnectionCloseDelayMillis(); + final long defaultHttp1ConnectionCloseDelayMillis = Flags.defaultHttp1ConnectionCloseDelayMillis(); assertThat(readDurationMillis).isBetween( defaultHttp1ConnectionCloseDelayMillis - 100, defaultHttp1ConnectionCloseDelayMillis + 1000 ); socket.close(); - Socket reuseSock = new Socket(); + final Socket reuseSock = new Socket(); reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort)); reuseSock.close(); } @@ -109,8 +108,8 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr @Test void shouldWaitForDisconnectByClientSideFirst() throws IOException { - Random random = new Random(); - short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); + final Random random = new Random(); + final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { socket.setReuseAddress(true); socket.setSoTimeout(1000); @@ -139,7 +138,7 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { assertThat(server.server().numConnections()).isEqualTo(1); socket.close(); - Socket reuseSock = new Socket(); + final Socket reuseSock = new Socket(); assertThatThrownBy(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) .isInstanceOf(BindException.class) .hasMessageContaining("Address already in use"); From f2c64e6a11f95bd4b8adb9951ffbc601b57acdc8 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 26 Apr 2024 12:31:39 +0900 Subject: [PATCH 12/24] test: remove setReuseAddress(true) from Http1ServerDelayedCloseConnectionTest --- .../server/Http1ServerDelayedCloseConnectionTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index 73fa6f8b6db..b60a4795c6a 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -17,6 +17,7 @@ package com.linecorp.armeria.server; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.io.BufferedReader; @@ -63,7 +64,6 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr final Random random = new Random(); final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { - socket.setReuseAddress(true); socket.setSoTimeout(100000); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); @@ -101,7 +101,8 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr socket.close(); final Socket reuseSock = new Socket(); - reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort)); + assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) + .doesNotThrowAnyException(); reuseSock.close(); } } @@ -111,7 +112,6 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { final Random random = new Random(); final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { - socket.setReuseAddress(true); socket.setSoTimeout(1000); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); From 29308ddf481c5eca97f73b07c80d3477389e1d3c Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 26 Apr 2024 13:25:28 +0900 Subject: [PATCH 13/24] test: add await().untilAsserted to wait for server to close the connection --- .../armeria/server/Http1ServerDelayedCloseConnectionTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index b60a4795c6a..0d665fd6fad 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.awaitility.Awaitility.await; import java.io.BufferedReader; import java.io.IOException; @@ -143,7 +144,7 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { .isInstanceOf(BindException.class) .hasMessageContaining("Address already in use"); reuseSock.close(); - assertThat(server.server().numConnections()).isZero(); + await().untilAsserted(() -> assertThat(server.server().numConnections()).isZero()); } } } From a0c178751493abf6c276147dc28ee05a102f53f2 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 26 Apr 2024 13:58:10 +0900 Subject: [PATCH 14/24] test: set socket.setReuseAddress(false) to Http1ServerDelayedCloseConnectionTest --- .../armeria/server/Http1ServerDelayedCloseConnectionTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index 0d665fd6fad..d8311360409 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -66,6 +66,7 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { socket.setSoTimeout(100000); + socket.setReuseAddress(false); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); writer.print("\r\n"); @@ -113,7 +114,8 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { final Random random = new Random(); final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { - socket.setSoTimeout(1000); + socket.setSoTimeout(100000); + socket.setReuseAddress(false); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); writer.print("\r\n"); From f966235394790afae82b154b2bf30795d01e45db Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 26 Apr 2024 15:39:20 +0900 Subject: [PATCH 15/24] test: set socket.setReuseAddress(false) to reuseSock in Http1ServerDelayedCloseConnectionTest --- .../armeria/server/Http1ServerDelayedCloseConnectionTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index d8311360409..1910388d3a6 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -66,7 +66,6 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { socket.setSoTimeout(100000); - socket.setReuseAddress(false); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); writer.print("\r\n"); @@ -103,6 +102,7 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr socket.close(); final Socket reuseSock = new Socket(); + reuseSock.setReuseAddress(false); assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) .doesNotThrowAnyException(); reuseSock.close(); @@ -115,7 +115,6 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { socket.setSoTimeout(100000); - socket.setReuseAddress(false); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); writer.print("\r\n"); @@ -142,6 +141,7 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { socket.close(); final Socket reuseSock = new Socket(); + reuseSock.setReuseAddress(false); assertThatThrownBy(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) .isInstanceOf(BindException.class) .hasMessageContaining("Address already in use"); From e0155d7b7f2ea3ba598d27be697e789867b199c0 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 26 Apr 2024 16:33:30 +0900 Subject: [PATCH 16/24] test: make the bind socket test run only in linux and macos --- ...Http1ServerDelayedCloseConnectionTest.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index 1910388d3a6..180b93030ba 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -32,6 +32,8 @@ import java.time.Duration; import java.util.Random; +import com.linecorp.armeria.common.util.OsType; +import com.linecorp.armeria.common.util.SystemInfo; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -100,12 +102,14 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr defaultHttp1ConnectionCloseDelayMillis + 1000 ); - socket.close(); - final Socket reuseSock = new Socket(); - reuseSock.setReuseAddress(false); - assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) - .doesNotThrowAnyException(); - reuseSock.close(); + // Additional test to check that the socket is in the CLOSED state + if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.WINDOWS) { + socket.close(); + try(Socket reuseSock = new Socket()) { + assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) + .doesNotThrowAnyException(); + } + } } } @@ -139,13 +143,15 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { assertThat(hasConnectionClose).isTrue(); assertThat(server.server().numConnections()).isEqualTo(1); - socket.close(); - final Socket reuseSock = new Socket(); - reuseSock.setReuseAddress(false); - assertThatThrownBy(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) - .isInstanceOf(BindException.class) - .hasMessageContaining("Address already in use"); - reuseSock.close(); + // Additional test to check that the socket is in the TIMED_WAIT state + if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.WINDOWS) { + socket.close(); + try(Socket reuseSock = new Socket()) { + assertThatThrownBy(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) + .isInstanceOf(BindException.class) + .hasMessageContaining("Address already in use"); + } + } await().untilAsserted(() -> assertThat(server.server().numConnections()).isZero()); } } From 5e25804d67d657927b0b372c1db70f5414ce474e Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Tue, 30 Apr 2024 13:11:36 +0900 Subject: [PATCH 17/24] test: make the bind socket test run only in linux and macos --- .../armeria/server/Http1ServerDelayedCloseConnectionTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index 180b93030ba..3e0cecdfde1 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -103,7 +103,7 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr ); // Additional test to check that the socket is in the CLOSED state - if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.WINDOWS) { + if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.MAC) { socket.close(); try(Socket reuseSock = new Socket()) { assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) @@ -144,7 +144,7 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { assertThat(server.server().numConnections()).isEqualTo(1); // Additional test to check that the socket is in the TIMED_WAIT state - if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.WINDOWS) { + if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.MAC) { socket.close(); try(Socket reuseSock = new Socket()) { assertThatThrownBy(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) From 151ff7a7c3834ef07fbc12543e1afc38b710c04a Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Fri, 3 May 2024 22:28:34 +0900 Subject: [PATCH 18/24] lint: apply code style --- .../armeria/server/HttpServerHandler.java | 8 ++++---- ...Http1ServerDelayedCloseConnectionTest.java | 19 ++++++++++--------- 2 files changed, 14 insertions(+), 13 deletions(-) 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 5ad7e65ff47..301fb0a2d36 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -796,10 +796,10 @@ private void handleRequestOrResponseComplete() { handledLastRequest = true; if (unfinishedRequests.isEmpty()) { ctx.channel().eventLoop().schedule(() -> { - if (ctx.channel().isActive()) { - ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); - } - }, Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); + if (ctx.channel().isActive()) { + ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); + } + }, Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); } } } diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index 3e0cecdfde1..ddc00edcbf3 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -54,10 +54,10 @@ protected void configure(ServerBuilder sb) { sb.tlsSelfSigned(); sb.service("/close", (ctx, req) -> { return HttpResponse.builder() - .ok() - .content("OK\n") - .header(HttpHeaderNames.CONNECTION, "close") - .build(); + .ok() + .content("OK\n") + .header(HttpHeaderNames.CONNECTION, "close") + .build(); }); } }; @@ -66,7 +66,7 @@ protected void configure(ServerBuilder sb) { void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() throws IOException { final Random random = new Random(); final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); - try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { + try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { socket.setSoTimeout(100000); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); @@ -105,7 +105,7 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr // Additional test to check that the socket is in the CLOSED state if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.MAC) { socket.close(); - try(Socket reuseSock = new Socket()) { + try (Socket reuseSock = new Socket()) { assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) .doesNotThrowAnyException(); } @@ -117,7 +117,7 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr void shouldWaitForDisconnectByClientSideFirst() throws IOException { final Random random = new Random(); final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); - try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { + try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { socket.setSoTimeout(100000); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); @@ -146,8 +146,9 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { // Additional test to check that the socket is in the TIMED_WAIT state if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.MAC) { socket.close(); - try(Socket reuseSock = new Socket()) { - assertThatThrownBy(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) + try (Socket reuseSock = new Socket()) { + assertThatThrownBy( + () -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) .isInstanceOf(BindException.class) .hasMessageContaining("Address already in use"); } From 12a536d518b2872e6e96f4236c5f77c34097a29d Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Mon, 6 May 2024 20:15:28 +0900 Subject: [PATCH 19/24] feat: make closing connection immediately when the response was already sent on HTTP1.1 --- .../com/linecorp/armeria/server/Http1RequestDecoder.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java b/core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java index a46616be294..edfea9224ae 100644 --- a/core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java +++ b/core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java @@ -321,7 +321,11 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception req = null; final boolean shouldReset; if (encoder instanceof ServerHttp1ObjectEncoder) { - keepAliveHandler.disconnectWhenFinished(); + if (encoder.isResponseHeadersSent(id, 1)) { + ctx.channel().close(); + } else { + keepAliveHandler.disconnectWhenFinished(); + } shouldReset = false; } else { // Upgraded to HTTP/2. Reset only if the remote peer is still open. From bbc35b710e8085142f25abd6955387c290820d9e Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Mon, 6 May 2024 22:24:37 +0900 Subject: [PATCH 20/24] feat: fix to not use random port --- ...Http1ServerDelayedCloseConnectionTest.java | 43 ++++++++----------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index ddc00edcbf3..fb2c1a4d753 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -30,10 +30,7 @@ import java.net.InetSocketAddress; import java.net.Socket; import java.time.Duration; -import java.util.Random; -import com.linecorp.armeria.common.util.OsType; -import com.linecorp.armeria.common.util.SystemInfo; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -64,10 +61,9 @@ protected void configure(ServerBuilder sb) { @Test void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() throws IOException { - final Random random = new Random(); - final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); - try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { + try (Socket socket = new Socket("127.0.0.1", server.httpPort())) { socket.setSoTimeout(100000); + final int socketPort = socket.getLocalPort(); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); writer.print("\r\n"); @@ -102,23 +98,19 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr defaultHttp1ConnectionCloseDelayMillis + 1000 ); - // Additional test to check that the socket is in the CLOSED state - if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.MAC) { - socket.close(); - try (Socket reuseSock = new Socket()) { - assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) - .doesNotThrowAnyException(); - } + socket.close(); + try (Socket reuseSock = new Socket()) { + assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, socketPort))) + .doesNotThrowAnyException(); } } } @Test void shouldWaitForDisconnectByClientSideFirst() throws IOException { - final Random random = new Random(); - final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); - try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { + try (Socket socket = new Socket("127.0.0.1", server.httpPort())) { socket.setSoTimeout(100000); + final int socketPort = socket.getLocalPort(); final PrintWriter writer = new PrintWriter(socket.getOutputStream()); writer.print("GET /close" + " HTTP/1.1\r\n"); writer.print("\r\n"); @@ -143,16 +135,15 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { assertThat(hasConnectionClose).isTrue(); assertThat(server.server().numConnections()).isEqualTo(1); - // Additional test to check that the socket is in the TIMED_WAIT state - if (SystemInfo.osType() == OsType.LINUX || SystemInfo.osType() == OsType.MAC) { - socket.close(); - try (Socket reuseSock = new Socket()) { - assertThatThrownBy( - () -> reuseSock.bind(new InetSocketAddress((InetAddress) null, localPort))) - .isInstanceOf(BindException.class) - .hasMessageContaining("Address already in use"); - } - } + socket.close(); + assertThatThrownBy( + () -> { + final Socket reuseSock = new Socket("127.0.0.1", server.httpPort(), null, socketPort); + reuseSock.close(); + }) + .isInstanceOf(BindException.class) + .hasMessageContaining("Address already in use"); + await().untilAsserted(() -> assertThat(server.server().numConnections()).isZero()); } } From 32cda15d49ed50ae1fcbb6bfa632d719fd9ffcd5 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Thu, 9 May 2024 21:07:30 +0900 Subject: [PATCH 21/24] test: refactor to read each line --- ...Http1ServerDelayedCloseConnectionTest.java | 44 +++++++------------ 1 file changed, 15 insertions(+), 29 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index fb2c1a4d753..0e37ccd2243 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -71,25 +71,17 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr final BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); assertThat(in.readLine()).isEqualTo("HTTP/1.1 200 OK"); - - String line; - boolean hasConnectionClose = false; - while ((line = in.readLine()) != null) { - if ("connection: close".equalsIgnoreCase(line)) { - hasConnectionClose = true; - } - if (line.isEmpty() || line.contains(":")) { - continue; - } - if (line.startsWith("OK")) { - break; - } - } + in.readLine(); // content-type + in.readLine(); // content-length + in.readLine(); // server + in.readLine(); // date + assertThat(in.readLine()).isEqualToIgnoringCase("connection: close"); + assertThat(in.readLine()).isEmpty(); + assertThat(in.readLine()).isEqualToIgnoringCase("OK"); final long readStartTimestamp = System.nanoTime(); final int readResult = in.read(); final long readDurationMillis = Duration.ofNanos(System.nanoTime() - readStartTimestamp).toMillis(); - assertThat(hasConnectionClose).isTrue(); assertThat(readResult).isEqualTo(-1); final long defaultHttp1ConnectionCloseDelayMillis = Flags.defaultHttp1ConnectionCloseDelayMillis(); @@ -118,27 +110,21 @@ void shouldWaitForDisconnectByClientSideFirst() throws IOException { final BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream())); assertThat(in.readLine()).isEqualTo("HTTP/1.1 200 OK"); + in.readLine(); // content-type + in.readLine(); // content-length + in.readLine(); // server + in.readLine(); // date + assertThat(in.readLine()).isEqualToIgnoringCase("connection: close"); + assertThat(in.readLine()).isEmpty(); + assertThat(in.readLine()).isEqualToIgnoringCase("OK"); - String line; - boolean hasConnectionClose = false; - while ((line = in.readLine()) != null) { - if ("connection: close".equalsIgnoreCase(line)) { - hasConnectionClose = true; - } - if (line.isEmpty() || line.contains(":")) { - continue; - } - if (line.startsWith("OK")) { - break; - } - } - assertThat(hasConnectionClose).isTrue(); assertThat(server.server().numConnections()).isEqualTo(1); socket.close(); assertThatThrownBy( () -> { final Socket reuseSock = new Socket("127.0.0.1", server.httpPort(), null, socketPort); + // close the socket in case initializing the socket doesn't throw an exception reuseSock.close(); }) .isInstanceOf(BindException.class) From 83d7ba2e1796911afcfbbd6b9a6b08f07596cdc7 Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Thu, 9 May 2024 21:25:03 +0900 Subject: [PATCH 22/24] docs: clarify the server closes the connection immediately when defaultHttp1ConnectionCloseDelayMillis is 0 --- core/src/main/java/com/linecorp/armeria/common/Flags.java | 4 ++-- .../main/java/com/linecorp/armeria/common/FlagsProvider.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/Flags.java b/core/src/main/java/com/linecorp/armeria/common/Flags.java index 43f54d8f6f3..822b1fd974d 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -1544,14 +1544,14 @@ public static DistributionStatisticConfig distributionStatisticConfig() { } /** - * Returns the default time in milliseconds to wait before closing an HTTP1 connection when a server needs + * Returns the default time in milliseconds to wait before closing an HTTP/1 connection when a server needs * to close the connection. This allows to avoid a server socket from remaining in the TIME_WAIT state * instead of CLOSED when a connection is closed. * *

The default value of this flag is * {@value DefaultFlagsProvider#DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS}. Specify the * {@code -Dcom.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis=} JVM option to - * override the default value.

+ * override the default value. {@code 0} closes the connection immediately.

*/ @UnstableApi public static long defaultHttp1ConnectionCloseDelayMillis() { diff --git a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java index 496d66dde3a..c5c2c027902 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -1161,14 +1161,14 @@ default DistributionStatisticConfig distributionStatisticConfig() { } /** - * Returns the default time in milliseconds to wait before closing an HTTP1 connection when a server needs + * Returns the default time in milliseconds to wait before closing an HTTP/1 connection when a server needs * to close the connection. This allows to avoid a server socket from remaining in the TIME_WAIT state * instead of CLOSED when a connection is closed. * *

The default value of this flag is * {@value DefaultFlagsProvider#DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS}. Specify the * {@code -Dcom.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis=} JVM option to - * override the default value.

+ * override the default value. {@code 0} closes the connection immediately.

*/ @Nullable @UnstableApi From 4a9496fc522f21f51bcd0c55ea75e7fca06cd33a Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Thu, 9 May 2024 21:26:27 +0900 Subject: [PATCH 23/24] feat: make the server close the connection immediately when defaultHttp1ConnectionCloseDelayMillis is 0 --- .../armeria/server/HttpServerHandler.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) 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 48fdd70cc18..777a5433843 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -793,11 +793,16 @@ private void handleRequestOrResponseComplete() { // Stop receiving new requests. handledLastRequest = true; if (unfinishedRequests.isEmpty()) { - ctx.channel().eventLoop().schedule(() -> { - if (ctx.channel().isActive()) { - ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); - } - }, Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); + final long closeDelay = Flags.defaultHttp1ConnectionCloseDelayMillis(); + if (closeDelay == 0) { + ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); + } else { + ctx.channel().eventLoop().schedule(() -> { + if (ctx.channel().isActive()) { + ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); + } + }, closeDelay, TimeUnit.MILLISECONDS); + } } } } From 3b45ee8fa624978fad5eed510410df426271f98e Mon Sep 17 00:00:00 2001 From: "soyun.seong" Date: Thu, 9 May 2024 21:31:27 +0900 Subject: [PATCH 24/24] test: increase the duration of the connection closure time --- .../armeria/server/Http1ServerDelayedCloseConnectionTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java index 0e37ccd2243..4096327c32d 100644 --- a/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -86,8 +86,8 @@ void shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose() thr final long defaultHttp1ConnectionCloseDelayMillis = Flags.defaultHttp1ConnectionCloseDelayMillis(); assertThat(readDurationMillis).isBetween( - defaultHttp1ConnectionCloseDelayMillis - 100, - defaultHttp1ConnectionCloseDelayMillis + 1000 + defaultHttp1ConnectionCloseDelayMillis - 2000, + defaultHttp1ConnectionCloseDelayMillis + 2000 ); socket.close();