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 ed92e07a81b..83dc3ff7bbf 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_UNLOGGED_EXCEPTIONS_REPORT_INTERVAL_MILLIS = 10000; + static final long DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS = 3000; private DefaultFlagsProvider() {} @@ -484,4 +485,9 @@ public Long defaultUnloggedExceptionsReportIntervalMillis() { 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 3d1250fd530..73165377cb6 100644 --- a/core/src/main/java/com/linecorp/armeria/common/Flags.java +++ b/core/src/main/java/com/linecorp/armeria/common/Flags.java @@ -421,6 +421,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 @@ -1563,6 +1567,21 @@ public static DistributionStatisticConfig distributionStatisticConfig() { return DISTRIBUTION_STATISTIC_CONFIG; } + /** + * 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. {@code 0} closes the connection immediately.

+ */ + @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 3a7f5227022..621d552d7a4 100644 --- a/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java +++ b/core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java @@ -1175,4 +1175,20 @@ default Long defaultUnloggedExceptionsReportIntervalMillis() { default DistributionStatisticConfig distributionStatisticConfig() { return null; } + + /** + * 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. {@code 0} closes the connection immediately.

+ */ + @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 18b99cb2eea..20fefc50b58 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"); + } + @Override public @Nullable Long defaultUnloggedExceptionsReportIntervalMillis() { return getLong("defaultUnloggedExceptionsReportIntervalMillis"); 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 b9712521a92..446c51f8bd8 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; } 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. 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 1bd85965164..777a5433843 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java @@ -40,6 +40,7 @@ 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; @@ -792,7 +793,16 @@ private void handleRequestOrResponseComplete() { // Stop receiving new requests. handledLastRequest = true; if (unfinishedRequests.isEmpty()) { - ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); + 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); + } } } } 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..4096327c32d --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java @@ -0,0 +1,136 @@ +/* + * 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 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; +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 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 { + + @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 { + 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"); + writer.flush(); + + 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"); + final long readStartTimestamp = System.nanoTime(); + final int readResult = in.read(); + final long readDurationMillis = Duration.ofNanos(System.nanoTime() - readStartTimestamp).toMillis(); + + assertThat(readResult).isEqualTo(-1); + + final long defaultHttp1ConnectionCloseDelayMillis = Flags.defaultHttp1ConnectionCloseDelayMillis(); + assertThat(readDurationMillis).isBetween( + defaultHttp1ConnectionCloseDelayMillis - 2000, + defaultHttp1ConnectionCloseDelayMillis + 2000 + ); + + socket.close(); + try (Socket reuseSock = new Socket()) { + assertThatCode(() -> reuseSock.bind(new InetSocketAddress((InetAddress) null, socketPort))) + .doesNotThrowAnyException(); + } + } + } + + @Test + void shouldWaitForDisconnectByClientSideFirst() throws IOException { + 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"); + writer.flush(); + + 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"); + + 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) + .hasMessageContaining("Address already in use"); + + await().untilAsserted(() -> assertThat(server.server().numConnections()).isZero()); + } + } +} 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..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; @@ -27,8 +29,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; -import com.google.common.io.ByteStreams; - import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.server.logging.AccessLogWriter; @@ -121,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); }