-
Notifications
You must be signed in to change notification settings - Fork 913
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
Close an HTTP/1.1 connection after delay #5616
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Left some minor comments 🙇
@@ -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(() -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ServiceRequestContext#executor
isn't necessarily the same as Channel#eventLoop
armeria/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java
Lines 375 to 387 in 1d268aa
final EventLoopGroup serviceWorkerGroup = serviceCfg.serviceWorkerGroup(); | |
if (serviceWorkerGroup == config.workerGroup()) { | |
serviceEventLoop = channelEventLoop; | |
needsDirectExecution = true; | |
} else { | |
serviceEventLoop = serviceWorkerGroup.next(); | |
needsDirectExecution = serviceEventLoop == channelEventLoop; | |
} | |
final DefaultServiceRequestContext reqCtx = new DefaultServiceRequestContext( | |
serviceCfg, channel, serviceEventLoop, config.meterRegistry(), protocol, | |
nextRequestId(routingCtx, serviceCfg), routingCtx, routingResult, req.exchangeType(), | |
req, sslSession, proxiedAddresses, clientAddress, remoteAddress, localAddress, | |
req.requestStartTimeNanos(), req.requestStartTimeMicros(), serviceCfg.contextHook()); |
We probably want to close from the channel's event loop (otherwise, an exception will be raised)
ctx.executor().schedule(() -> | |
ctx.channel().eventLoop().schedule(() -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I missed it. Thanks for letting me know.
I fixed it to use the channel's event loop.
ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); | ||
ctx.executor().schedule(() -> | ||
ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE), | ||
1000, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional nit; using seconds is probably slightly easier to reason about
1000, TimeUnit.MILLISECONDS); | |
1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this value can be configured in milliseconds on fast networks.
#5616 (comment) |
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we check if the channel is still active? Meanwhile, the channel may have been closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking! I added to check ctx.channel().isActive()
before executing the close task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a test case. You might want to implement a simple low-level HTTP/1 client in your test to know the socket-level timings.
I'd recommend checking out the source code of Http1ServerKeepAliveTest
to get the basic ideas.
42f87ea
to
4a7f0ef
Compare
…or the server to close the connection
…layedCloseConnectionTest
Looking into the failing test, I guess there is a case where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Left a few style related comments 🙇
ctx.channel().eventLoop().schedule(() -> { | ||
if (ctx.channel().isActive()) { | ||
ctx.writeAndFlush(Unpooled.EMPTY_BUFFER).addListener(CLOSE); | ||
} | ||
}, Flags.defaultHttp1ConnectionCloseDelayMillis(), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you align these lines? (you may also want to check if code style is correctly set in your IDE ref: https://armeria.dev/community/developer-guide#setting-up-your-ide)
ctx.channel().eventLoop().schedule(() -> { | |
if (ctx.channel().isActive()) { | |
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know. I have checked that the code style setting was not set correctly.
I have fixed the code above.
@@ -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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
} | ||
|
||
@Test | ||
void shouldWaitForDisconnectByClientSideFirst() throws IOException { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); | ||
try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's 1) overkill to check the socket state as this is out of our control 2) binding to a random socket introduces unnecessary flakiness.
Instead of testing the OS's behavior of managing TCP states, what do you think of sticking to testing the behavior of the server-side closing the connection? (So what do you think of removing the localPort
related logic?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the purpose of this PR is to prevent the server's socket state from getting in TIME_WAIT by waiting for the client to close the connection first. Therefore, I thought it was necessary to check which side closed the connection, and as a way to check this behaviour I thought it was meaningful to try to reuse the client socket and check which side performed the close first through immediate reusability.
In order to try to reuse the client socket, it is necessary to test by setting the port number directly instead of using the port number assigned by the OS, so I implemented the test by setting the localPort number.
I agree with the risk of using a random port number as you mentioned, and I found there is a function socket.getLocalPort()
. So I fixed it to reuse the port number assigned from the OS.
However, if checking the socket state like this is unnecessary, I would remove the part about checking if the socket is in TIME_WAIT or CLOSE state through socket reuse!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as long as the test consistently passes it is OK to keep the behavior 👍
Went through the code, and I think the issue is the following:
Overall, I think it's probably fine to just close the connection immediately if we can't write the 413 response headers. This isn't a perfect solution, but at least preserves backwards compatibility.
|
When also I analysed the reason for the failure of this test, I found that the response is sent immediately at 404. And when the content size exceeds |
fd7e68e
to
bbc35b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for merging to me. Thanks for tackling this difficult issue @dachshu! 🙇 👍 🙇
@@ -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( |
There was a problem hiding this comment.
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.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
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(); | |
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to read reads each line!
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; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, can we just explicitly check each line?
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; | |
} | |
} | |
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored it too 🙇
} | ||
|
||
@Test | ||
void shouldWaitForDisconnectByClientSideFirst() throws IOException { |
There was a problem hiding this comment.
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 👍
assertThatThrownBy( | ||
() -> { | ||
final Socket reuseSock = new Socket("127.0.0.1", server.httpPort(), null, socketPort); | ||
reuseSock.close(); |
There was a problem hiding this comment.
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
reuseSock.close(); | |
// close the socket in case initializing the socket doesn't throw an exception | |
reuseSock.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for good suggestions 🙇
final short localPort = (short) random.nextInt(Short.MAX_VALUE + 1); | ||
try (Socket socket = new Socket("127.0.0.1", server.httpPort(), null, localPort)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as long as the test consistently passes it is OK to keep the behavior 👍
Thanks! Let us know if you would like to try handling this as a follow-up issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Left minor comments.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for helping to correct 🙇
core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left just small suggestions. Thanks!
core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/server/Http1ServerDelayedCloseConnectionTest.java
Outdated
Show resolved
Hide resolved
Yes, I would like to! |
…ltHttp1ConnectionCloseDelayMillis is 0
…tp1ConnectionCloseDelayMillis is 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me! 👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks a lot, @dachshu! 👍 👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @dachshu! 🙇♂️👍
Motivation: - Closes line#4849 - To suppress a server socket from remaining in the `TIME_WAIT` state instead of `CLOSED` when a connection is closed. Modifications: - Remove the force shutdown mode of `AbstractHttpResponseHandler` that is activated when a user sets the `Connection: close` HTTP header - Create a schedule that waits and closes the connection to wait for the client-side connection to close, rather than closing the HTTP/1.1 connection immediately. Result: - Closes line#4849 - When a user wants to close an HTTP/1.1 server connection, the armeria server first waits for the client to close the connection for a while to make the connection state to `CLOSED` instead of `TIME_WAIT`.
Motivation:
TIME_WAIT
state instead ofCLOSED
when a connection is closed.Modifications:
AbstractHttpResponseHandler
that is activated when a user sets theConnection: close
HTTP headerResult:
CLOSED
instead ofTIME_WAIT
.