From f87a81b8140c299b3ac8e212aad8f7c372804fca Mon Sep 17 00:00:00 2001 From: ooeunz Date: Wed, 27 Nov 2024 14:50:47 +0900 Subject: [PATCH 1/7] Add option to close connections with RST on timeout - Introduced a configuration option `useRstOnTimeout` in `Http1Config` to allow connections to be closed with an RST (Reset) signal when a timeout occurs. - Updated `HttpRequestExecutor` to respect this configuration and invoke the appropriate close mode (`CloseMode.IMMEDIATE` for RST or `Closer.closeQuietly` for FIN). - Maintained backward compatibility by defaulting to FIN-based connection closure. This change improves flexibility and allows faster resource cleanup in timeout scenarios, aligning with the HTTP/1.1 recommendations for abnormal connection termination. Resolves: HTTPCLIENT-2324 --- .../hc/core5/http/config/Http1Config.java | 20 +++++++++++++++++-- .../http/impl/io/HttpRequestExecutor.java | 7 ++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java b/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java index 015162b371..81ec83ac96 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java @@ -56,10 +56,11 @@ public class Http1Config { private final int maxHeaderCount; private final int maxEmptyLineCount; private final int initialWindowSize; + private final boolean useRstOnTimeout; Http1Config(final HttpVersion version, final int bufferSize, final int chunkSizeHint, final Timeout waitForContinueTimeout, final int maxLineLength, final int maxHeaderCount, - final int maxEmptyLineCount, final int initialWindowSize) { + final int maxEmptyLineCount, final int initialWindowSize, final boolean useRstOnTimeout) { super(); this.version = version; this.bufferSize = bufferSize; @@ -69,6 +70,7 @@ public class Http1Config { this.maxHeaderCount = maxHeaderCount; this.maxEmptyLineCount = maxEmptyLineCount; this.initialWindowSize = initialWindowSize; + this.useRstOnTimeout = useRstOnTimeout; } /** @@ -109,6 +111,10 @@ public int getInitialWindowSize() { return initialWindowSize; } + public boolean getUseRstOnTimeout() { + return useRstOnTimeout; + } + @Override public String toString() { final StringBuilder builder = new StringBuilder(); @@ -120,6 +126,7 @@ public String toString() { .append(", maxHeaderCount=").append(maxHeaderCount) .append(", maxEmptyLineCount=").append(maxEmptyLineCount) .append(", initialWindowSize=").append(initialWindowSize) + .append(", useRstOnTimeout=").append(useRstOnTimeout) .append("]"); return builder.toString(); } @@ -147,6 +154,7 @@ public static Http1Config.Builder copy(final Http1Config config) { private static final int INIT_MAX_HEADER_COUNT = -1; private static final int INIT_MAX_LINE_LENGTH = -1; private static final int INIT_MAX_EMPTY_LINE_COUNT = 10; + private static final boolean USE_RST_ON_TIMEOUT = false; public static class Builder { @@ -158,6 +166,7 @@ public static class Builder { private int maxHeaderCount; private int maxEmptyLineCount; private int initialWindowSize; + private boolean userRstOnTimeout; Builder() { this.version = HttpVersion.HTTP_1_1; @@ -168,6 +177,7 @@ public static class Builder { this.maxHeaderCount = INIT_MAX_HEADER_COUNT; this.maxEmptyLineCount = INIT_MAX_EMPTY_LINE_COUNT; this.initialWindowSize = INIT_WINDOW_SIZE; + this.userRstOnTimeout = USE_RST_ON_TIMEOUT; } /** @@ -222,6 +232,11 @@ public Builder setInitialWindowSize(final int initialWindowSize) { return this; } + public Builder setUserRstOnTimeout(final boolean userRstOnTimeout) { + this.userRstOnTimeout = userRstOnTimeout; + return this; + } + public Http1Config build() { return new Http1Config( version, @@ -231,7 +246,8 @@ public Http1Config build() { maxLineLength, maxHeaderCount, maxEmptyLineCount, - initialWindowSize); + initialWindowSize, + userRstOnTimeout); } } diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java index a75ca65ba1..79d955e862 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java @@ -54,6 +54,7 @@ import org.apache.hc.core5.http.protocol.HttpContext; import org.apache.hc.core5.http.protocol.HttpCoreContext; import org.apache.hc.core5.http.protocol.HttpProcessor; +import org.apache.hc.core5.io.CloseMode; import org.apache.hc.core5.io.Closer; import org.apache.hc.core5.util.Args; import org.apache.hc.core5.util.Timeout; @@ -212,7 +213,11 @@ public ClassicHttpResponse execute( return response; } catch (final HttpException | IOException | RuntimeException ex) { - Closer.closeQuietly(conn); + if (http1Config.getUseRstOnTimeout()) { + Closer.close(conn, CloseMode.IMMEDIATE); + } else { + Closer.closeQuietly(conn); + } throw ex; } } From c66648c9ae8fb4cfafe1ff8056eff80c9b89d922 Mon Sep 17 00:00:00 2001 From: ooeunz Date: Wed, 27 Nov 2024 16:57:47 +0900 Subject: [PATCH 2/7] Add version tag --- .../java/org/apache/hc/core5/http/config/Http1Config.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java b/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java index 81ec83ac96..0fe7ea38b1 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java @@ -111,6 +111,9 @@ public int getInitialWindowSize() { return initialWindowSize; } + /** + * @since 5.4 + */ public boolean getUseRstOnTimeout() { return useRstOnTimeout; } @@ -232,6 +235,9 @@ public Builder setInitialWindowSize(final int initialWindowSize) { return this; } + /** + * @since 5.4 + */ public Builder setUserRstOnTimeout(final boolean userRstOnTimeout) { this.userRstOnTimeout = userRstOnTimeout; return this; From 0c2c7224bd8fb54996deb563519622c48bfc0aa0 Mon Sep 17 00:00:00 2001 From: ooeunz Date: Wed, 27 Nov 2024 19:32:41 +0900 Subject: [PATCH 3/7] Change default behavior to IMMEDIATE close --- .../hc/core5/http/config/Http1Config.java | 26 ++----------------- .../http/impl/io/HttpRequestExecutor.java | 6 +---- .../http/impl/io/TestHttpRequestExecutor.java | 7 +++-- 3 files changed, 8 insertions(+), 31 deletions(-) diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java b/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java index 0fe7ea38b1..015162b371 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java @@ -56,11 +56,10 @@ public class Http1Config { private final int maxHeaderCount; private final int maxEmptyLineCount; private final int initialWindowSize; - private final boolean useRstOnTimeout; Http1Config(final HttpVersion version, final int bufferSize, final int chunkSizeHint, final Timeout waitForContinueTimeout, final int maxLineLength, final int maxHeaderCount, - final int maxEmptyLineCount, final int initialWindowSize, final boolean useRstOnTimeout) { + final int maxEmptyLineCount, final int initialWindowSize) { super(); this.version = version; this.bufferSize = bufferSize; @@ -70,7 +69,6 @@ public class Http1Config { this.maxHeaderCount = maxHeaderCount; this.maxEmptyLineCount = maxEmptyLineCount; this.initialWindowSize = initialWindowSize; - this.useRstOnTimeout = useRstOnTimeout; } /** @@ -111,13 +109,6 @@ public int getInitialWindowSize() { return initialWindowSize; } - /** - * @since 5.4 - */ - public boolean getUseRstOnTimeout() { - return useRstOnTimeout; - } - @Override public String toString() { final StringBuilder builder = new StringBuilder(); @@ -129,7 +120,6 @@ public String toString() { .append(", maxHeaderCount=").append(maxHeaderCount) .append(", maxEmptyLineCount=").append(maxEmptyLineCount) .append(", initialWindowSize=").append(initialWindowSize) - .append(", useRstOnTimeout=").append(useRstOnTimeout) .append("]"); return builder.toString(); } @@ -157,7 +147,6 @@ public static Http1Config.Builder copy(final Http1Config config) { private static final int INIT_MAX_HEADER_COUNT = -1; private static final int INIT_MAX_LINE_LENGTH = -1; private static final int INIT_MAX_EMPTY_LINE_COUNT = 10; - private static final boolean USE_RST_ON_TIMEOUT = false; public static class Builder { @@ -169,7 +158,6 @@ public static class Builder { private int maxHeaderCount; private int maxEmptyLineCount; private int initialWindowSize; - private boolean userRstOnTimeout; Builder() { this.version = HttpVersion.HTTP_1_1; @@ -180,7 +168,6 @@ public static class Builder { this.maxHeaderCount = INIT_MAX_HEADER_COUNT; this.maxEmptyLineCount = INIT_MAX_EMPTY_LINE_COUNT; this.initialWindowSize = INIT_WINDOW_SIZE; - this.userRstOnTimeout = USE_RST_ON_TIMEOUT; } /** @@ -235,14 +222,6 @@ public Builder setInitialWindowSize(final int initialWindowSize) { return this; } - /** - * @since 5.4 - */ - public Builder setUserRstOnTimeout(final boolean userRstOnTimeout) { - this.userRstOnTimeout = userRstOnTimeout; - return this; - } - public Http1Config build() { return new Http1Config( version, @@ -252,8 +231,7 @@ public Http1Config build() { maxLineLength, maxHeaderCount, maxEmptyLineCount, - initialWindowSize, - userRstOnTimeout); + initialWindowSize); } } diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java index 79d955e862..beac6bf385 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java @@ -213,11 +213,7 @@ public ClassicHttpResponse execute( return response; } catch (final HttpException | IOException | RuntimeException ex) { - if (http1Config.getUseRstOnTimeout()) { - Closer.close(conn, CloseMode.IMMEDIATE); - } else { - Closer.closeQuietly(conn); - } + Closer.close(conn, CloseMode.IMMEDIATE); throw ex; } } diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java index d961c5a283..0be095f9b8 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java @@ -28,6 +28,7 @@ package org.apache.hc.core5.http.impl.io; import java.io.IOException; +import java.net.SocketTimeoutException; import java.util.List; import org.apache.hc.core5.http.ClassicHttpRequest; @@ -37,12 +38,14 @@ import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.Method; +import org.apache.hc.core5.http.config.Http1Config; import org.apache.hc.core5.http.io.HttpClientConnection; import org.apache.hc.core5.http.io.HttpResponseInformationCallback; import org.apache.hc.core5.http.message.BasicClassicHttpRequest; import org.apache.hc.core5.http.message.BasicClassicHttpResponse; import org.apache.hc.core5.http.protocol.HttpCoreContext; import org.apache.hc.core5.http.protocol.HttpProcessor; +import org.apache.hc.core5.io.CloseMode; import org.apache.hc.core5.util.Timeout; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -402,7 +405,7 @@ void testExecutionIOException() throws Exception { Mockito.doThrow(new IOException("Oopsie")).when(conn).sendRequestHeader(request); Assertions.assertThrows(IOException.class, () -> executor.execute(request, conn, context)); - Mockito.verify(conn).close(); + Mockito.verify(conn).close(CloseMode.IMMEDIATE); } @Test @@ -415,7 +418,7 @@ void testExecutionRuntimeException() throws Exception { Mockito.doThrow(new RuntimeException("Oopsie")).when(conn).receiveResponseHeader(); Assertions.assertThrows(RuntimeException.class, () -> executor.execute(request, conn, context)); - Mockito.verify(conn).close(); + Mockito.verify(conn).close(CloseMode.IMMEDIATE); } } From 1b2709a215d25fa87496ef4315c5111eb2ba7d67 Mon Sep 17 00:00:00 2001 From: ooeunz Date: Wed, 27 Nov 2024 20:18:30 +0900 Subject: [PATCH 4/7] Separate connection close handling for IOException and regular exceptions --- .../http/impl/io/HttpRequestExecutor.java | 7 ++++++- .../http/impl/io/TestHttpRequestExecutor.java | 21 ++++++++++++------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java index beac6bf385..d5afa1cfd6 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java @@ -59,6 +59,8 @@ import org.apache.hc.core5.util.Args; import org.apache.hc.core5.util.Timeout; +import javax.net.ssl.SSLException; + /** * {@code HttpRequestExecutor} is a client side HTTP protocol handler based * on the blocking (classic) I/O model. @@ -212,9 +214,12 @@ public ClassicHttpResponse execute( } return response; - } catch (final HttpException | IOException | RuntimeException ex) { + } catch (final IOException | RuntimeException ex) { Closer.close(conn, CloseMode.IMMEDIATE); throw ex; + } catch (final HttpException ex) { + Closer.closeQuietly(conn); + throw ex; } } diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java index 0be095f9b8..d2179a0286 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java @@ -31,13 +31,7 @@ import java.net.SocketTimeoutException; import java.util.List; -import org.apache.hc.core5.http.ClassicHttpRequest; -import org.apache.hc.core5.http.ClassicHttpResponse; -import org.apache.hc.core5.http.HeaderElements; -import org.apache.hc.core5.http.HttpEntity; -import org.apache.hc.core5.http.HttpHeaders; -import org.apache.hc.core5.http.HttpResponse; -import org.apache.hc.core5.http.Method; +import org.apache.hc.core5.http.*; import org.apache.hc.core5.http.config.Http1Config; import org.apache.hc.core5.http.io.HttpClientConnection; import org.apache.hc.core5.http.io.HttpResponseInformationCallback; @@ -421,4 +415,17 @@ void testExecutionRuntimeException() throws Exception { Mockito.verify(conn).close(CloseMode.IMMEDIATE); } + @Test + void testExecutionHttpException() throws Exception { + final HttpClientConnection conn = Mockito.mock(HttpClientConnection.class); + final HttpRequestExecutor executor = new HttpRequestExecutor(); + + final HttpCoreContext context = HttpCoreContext.create(); + final ClassicHttpRequest request = new BasicClassicHttpRequest(Method.GET, "/"); + + Mockito.doThrow(new HttpException("Oopsie")).when(conn).receiveResponseHeader(); + Assertions.assertThrows(HttpException.class, () -> executor.execute(request, conn, context)); + Mockito.verify(conn).close(); + } + } From 4bd75b90a50c5d98858c2797453d1e5ec12e0027 Mon Sep 17 00:00:00 2001 From: ooeunz Date: Thu, 28 Nov 2024 02:11:48 +0900 Subject: [PATCH 5/7] Use closeQuietly for SSLException handling --- .../apache/hc/core5/http/impl/io/HttpRequestExecutor.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java index d5afa1cfd6..9bff2058ef 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java +++ b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java @@ -214,12 +214,12 @@ public ClassicHttpResponse execute( } return response; + } catch (final HttpException | SSLException ex) { + Closer.closeQuietly(conn); + throw ex; } catch (final IOException | RuntimeException ex) { Closer.close(conn, CloseMode.IMMEDIATE); throw ex; - } catch (final HttpException ex) { - Closer.closeQuietly(conn); - throw ex; } } From 7247895290cb1b5f22c14ad84f78755971fbfbdf Mon Sep 17 00:00:00 2001 From: ooeunz Date: Thu, 28 Nov 2024 06:17:42 +0900 Subject: [PATCH 6/7] Fix code style --- .../hc/core5/http/impl/io/TestHttpRequestExecutor.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java index d2179a0286..c8ca86aa4c 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java @@ -27,12 +27,7 @@ package org.apache.hc.core5.http.impl.io; -import java.io.IOException; -import java.net.SocketTimeoutException; -import java.util.List; - import org.apache.hc.core5.http.*; -import org.apache.hc.core5.http.config.Http1Config; import org.apache.hc.core5.http.io.HttpClientConnection; import org.apache.hc.core5.http.io.HttpResponseInformationCallback; import org.apache.hc.core5.http.message.BasicClassicHttpRequest; @@ -47,6 +42,9 @@ import org.mockito.ArgumentMatchers; import org.mockito.Mockito; +import java.io.IOException; +import java.util.List; + class TestHttpRequestExecutor { @Test From d24383370a714e87433ff290a29fbfa4ceb2ecd0 Mon Sep 17 00:00:00 2001 From: ooeunz Date: Thu, 28 Nov 2024 06:40:41 +0900 Subject: [PATCH 7/7] Replace wildcard imports with explicit imports --- .../hc/core5/http/impl/io/TestHttpRequestExecutor.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java index c8ca86aa4c..f9451bf828 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java +++ b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/io/TestHttpRequestExecutor.java @@ -27,7 +27,14 @@ package org.apache.hc.core5.http.impl.io; -import org.apache.hc.core5.http.*; +import org.apache.hc.core5.http.ClassicHttpRequest; +import org.apache.hc.core5.http.ClassicHttpResponse; +import org.apache.hc.core5.http.HeaderElements; +import org.apache.hc.core5.http.HttpEntity; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpResponse; +import org.apache.hc.core5.http.Method; +import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.io.HttpClientConnection; import org.apache.hc.core5.http.io.HttpResponseInformationCallback; import org.apache.hc.core5.http.message.BasicClassicHttpRequest;