Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close an HTTP/1.1 connection after delay #5616

Merged
merged 28 commits into from
May 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
5b6a9fd
refactor: remove the Force shutdown mode
dachshu Apr 12, 2024
f01ca8a
feat: create a schedule for waiting and closing in HTTP1
dachshu Apr 12, 2024
824bb23
feat: define a defaultHttp1ConnectionCloseDelayMillis Flag
dachshu Apr 23, 2024
1d764aa
refactor: replace hardcoded defaultHttp1ConnectionCloseDelayMillis wi…
dachshu Apr 23, 2024
5820fe9
fix: to schedule the close connection task to run in the channel's ev…
dachshu Apr 23, 2024
4a7f0ef
refactor: fix import order
dachshu Apr 24, 2024
90b5509
test: set defaultHttp1ConnectionCloseDelayMillis to 0 on HttpServerPa…
dachshu Apr 24, 2024
ae86f29
feat: check the active status of a channel before running a close task
dachshu Apr 24, 2024
e74d182
test: read only a first line in HttpServerPathTest to does not wait f…
dachshu Apr 25, 2024
4ec928d
test: implement test cases for Http1ServerDelayedCloseConnectionTest
dachshu Apr 26, 2024
abbfc56
test: fix lint errors
dachshu Apr 26, 2024
f2c64e6
test: remove setReuseAddress(true) from Http1ServerDelayedCloseConnec…
dachshu Apr 26, 2024
29308dd
test: add await().untilAsserted to wait for server to close the conne…
dachshu Apr 26, 2024
a0c1787
test: set socket.setReuseAddress(false) to Http1ServerDelayedCloseCon…
dachshu Apr 26, 2024
f966235
test: set socket.setReuseAddress(false) to reuseSock in Http1ServerDe…
dachshu Apr 26, 2024
38d644e
Merge branch 'main' into ISSUE-4849-close-connection-after-delay
dachshu Apr 26, 2024
e0155d7
test: make the bind socket test run only in linux and macos
dachshu Apr 26, 2024
5e25804
test: make the bind socket test run only in linux and macos
dachshu Apr 30, 2024
151ff7a
lint: apply code style
dachshu May 3, 2024
12a536d
feat: make closing connection immediately when the response was alrea…
dachshu May 6, 2024
bbc35b7
feat: fix to not use random port
dachshu May 6, 2024
cd2d1a4
Merge branch 'main' into ISSUE-4849-close-connection-after-delay
dachshu May 7, 2024
32cda15
test: refactor to read each line
dachshu May 9, 2024
83d7ba2
docs: clarify the server closes the connection immediately when defau…
dachshu May 9, 2024
4a9496f
feat: make the server close the connection immediately when defaultHt…
dachshu May 9, 2024
3b45ee8
test: increase the duration of the connection closure time
dachshu May 9, 2024
51986e3
Merge branch 'main' into ISSUE-4849-close-connection-after-delay
dachshu May 10, 2024
596b223
Merge branch 'main' into ISSUE-4849-close-connection-after-delay
dachshu May 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}

Expand Down Expand Up @@ -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;
}
}
19 changes: 19 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/Flags.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
*
* <p>The default value of this flag is
* {@value DefaultFlagsProvider#DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS}. Specify the
* {@code -Dcom.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis=<long>} JVM option to
* override the default value. {@code 0} closes the connection immediately.</p>
*/
@UnstableApi
public static long defaultHttp1ConnectionCloseDelayMillis() {
return DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS;
}

@Nullable
private static String nullableCaffeineSpec(Function<FlagsProvider, String> method, String flagName) {
return caffeineSpec(method, flagName, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>The default value of this flag is
* {@value DefaultFlagsProvider#DEFAULT_HTTP1_CONNECTION_CLOSE_DELAY_MILLIS}. Specify the
* {@code -Dcom.linecorp.armeria.defaultHttp1ConnectionCloseDelayMillis=<long>} JVM option to
* override the default value. {@code 0} closes the connection immediately. </p>
*/
@Nullable
@UnstableApi
default Long defaultHttp1ConnectionCloseDelayMillis() {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ abstract class AbstractHttpResponseHandler {

private final CompletableFuture<Void> completionFuture;
private boolean isComplete;
private boolean needsDisconnection;

AbstractHttpResponseHandler(ChannelHandlerContext ctx,
ServerHttpObjectEncoder responseEncoder,
Expand All @@ -77,7 +76,6 @@ boolean isDone() {
}

void disconnectWhenFinished() {
needsDisconnection = true;
responseEncoder.keepAliveHandler().disconnectWhenFinished();
}

Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question) I didn't understand the intention of this test. If the functionality isn't very different from shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose, what do you think of just removing it?

Copy link
Contributor Author

@dachshu dachshu May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shouldWaitForDisconnectByClientSideFirst test checks if the server sends the Connection: close header and waits for the client to close the connection first, rather than closing the connection immediately. It additionally checks if the server closes the connection after the client closes it.
On the other hand, the shouldDelayDisconnectByServerSideIfClientDoesNotHandleConnectionClose test verifies that if the client receives the Connection: close header but ignores it without attempting to close the connection, and the close connection task scheduled by the server executes well waiting for the set time on the sever side before closing the connection.
Therefore, I wrote two tests because I think the purpose of the two tests and what they verify are different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I think I missed that point in the last review cycle. Thanks for the explanation 👍

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that we know why this logic exists

Suggested change
reuseSock.close();
// close the socket in case initializing the socket doesn't throw an exception
reuseSock.close();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for good suggestions 🙇

})
.isInstanceOf(BindException.class)
.hasMessageContaining("Address already in use");

await().untilAsserted(() -> assertThat(server.server().numConnections()).isZero());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question) I think the previous approach of collecting all bytes was correct (in the off-chance that the bytes are fragmented). What do you think of reverting this file?

Copy link
Contributor Author

@dachshu dachshu May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the following implementation, the ByteStreams.toByteArray(s.getInputStream()) call returns the value when the inputStream is closed which is only happened when the connection is closed from the server.
https://github.com/google/guava/blob/master/guava/src/com/google/common/io/ByteStreams.java#L195~L198

With this PR, I changed the server to wait for the client to close first instead of closing the connection immediately. However in this test, the client receives the Connection: Close header from the server but ignores it. So the test waits for the server to close the connction, causing the test timeout.
I tried to adjust the defaultHttp1ConnectionCloseDelayMillis using @SetSystemProperty or System.setProperty() to reduce the test time, but it didn't work well in shadedTest 😭
Since only the first line is needed for this test validation, I changed it to read only the first line and check the result to make the test take less time.
If it is a problem, I would consider reducing the defaultHttp1ConnectionCloseDelayMillis.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I think I missed this point. I think it's fine to leave as-is then. Thanks for the explanation 👍

I tried to adjust the defaultHttp1ConnectionCloseDelayMillis using @SetSystemProperty or System.setProperty() to reduce the test time, but it didn't work well in shadedTest

I assume this is because Flags is already initialized when other tests are run or ServerExtension is constructed.

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);
}
Expand Down
Loading