From b87e9287e1262585d9ac65e0cd7f3de9d1b099e8 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Wed, 23 Oct 2024 15:26:05 +0200 Subject: [PATCH 1/3] [MRESOLVER-612] Align timeout interpretations across HTTP transports The `ConfigurationProperties#REQUEST_TIMEOUT` was wrongly interpreted by JDK and Jetty clients as "absolute max duration of HTTP request". Also, up default CONNECT_TIMEOUT from 10sec to 30sec. --- .../aether/ConfigurationProperties.java | 2 +- .../transport/apache/ApacheTransporter.java | 10 +++-- .../aether/transport/jdk/JdkTransporter.java | 37 ++++++++++--------- .../transport/jdk/JdkTransporterTest.java | 5 +++ .../transport/jetty/JettyTransporter.java | 24 ++++++------ 5 files changed, 43 insertions(+), 35 deletions(-) diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java b/maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java index 0179733ae..9ee3ccf1d 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java @@ -215,7 +215,7 @@ public final class ConfigurationProperties { /** * The default connect timeout to use if {@link #CONNECT_TIMEOUT} isn't set. */ - public static final int DEFAULT_CONNECT_TIMEOUT = 10 * 1000; + public static final int DEFAULT_CONNECT_TIMEOUT = 30 * 1000; /** * The maximum amount of time (in milliseconds) to wait for remaining data to arrive from a remote server. Note that diff --git a/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java b/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java index cb54e4676..73d881d56 100644 --- a/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java +++ b/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java @@ -288,16 +288,20 @@ final class ApacheTransporter extends AbstractTransporter implements HttpTranspo .register(AuthSchemes.KERBEROS, new KerberosSchemeFactory()) .build(); SocketConfig socketConfig = - SocketConfig.custom().setSoTimeout(requestTimeout).build(); + // the time to establish connection (low level) + SocketConfig.custom().setSoTimeout(connectTimeout).build(); RequestConfig requestConfig = RequestConfig.custom() .setMaxRedirects(maxRedirects) .setRedirectsEnabled(followRedirects) .setRelativeRedirectsAllowed(followRedirects) + // the time waiting for data; max time between two data packets + .setSocketTimeout(requestTimeout) + // the time to establish the connection (high level) .setConnectTimeout(connectTimeout) - .setConnectionRequestTimeout(connectTimeout) + // the time to wait for a connection from the connection manager/pool + .setConnectionRequestTimeout(requestTimeout) .setLocalAddress(getHttpLocalAddress(session, repository)) .setCookieSpec(CookieSpecs.STANDARD) - .setSocketTimeout(requestTimeout) .build(); HttpRequestRetryHandler retryHandler; diff --git a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java index 1f835c0ea..b3cbf8b46 100644 --- a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java +++ b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java @@ -99,6 +99,11 @@ /** * JDK Transport using {@link HttpClient}. + *

+ * Known issues: + *

* * @since 2.0.0 */ @@ -122,6 +127,8 @@ final class JdkTransporter extends AbstractTransporter implements HttpTransporte private final Map headers; + private final int connectTimeout; + private final int requestTimeout; private final Boolean expectContinue; @@ -177,6 +184,11 @@ final class JdkTransporter extends AbstractTransporter implements HttpTransporte } headers.put(CACHE_CONTROL, "no-cache, no-store"); + this.connectTimeout = ConfigUtils.getInteger( + session, + ConfigurationProperties.DEFAULT_CONNECT_TIMEOUT, + ConfigurationProperties.CONNECT_TIMEOUT + "." + repository.getId(), + ConfigurationProperties.CONNECT_TIMEOUT); this.requestTimeout = ConfigUtils.getInteger( session, ConfigurationProperties.DEFAULT_REQUEST_TIMEOUT, @@ -244,10 +256,8 @@ public int classify(Throwable error) { @Override protected void implPeek(PeekTask task) throws Exception { - HttpRequest.Builder request = HttpRequest.newBuilder() - .uri(resolve(task)) - .timeout(Duration.ofMillis(requestTimeout)) - .method("HEAD", HttpRequest.BodyPublishers.noBody()); + HttpRequest.Builder request = + HttpRequest.newBuilder().uri(resolve(task)).method("HEAD", HttpRequest.BodyPublishers.noBody()); headers.forEach(request::setHeader); try { HttpResponse response = send(request.build(), HttpResponse.BodyHandlers.discarding()); @@ -266,10 +276,8 @@ protected void implGet(GetTask task) throws Exception { try { while (true) { - HttpRequest.Builder request = HttpRequest.newBuilder() - .uri(resolve(task)) - .timeout(Duration.ofMillis(requestTimeout)) - .method("GET", HttpRequest.BodyPublishers.noBody()); + HttpRequest.Builder request = + HttpRequest.newBuilder().uri(resolve(task)).method("GET", HttpRequest.BodyPublishers.noBody()); headers.forEach(request::setHeader); if (resume) { @@ -385,8 +393,7 @@ private void closeBody(HttpResponse streamHttpResponse) throws IOEx @Override protected void implPut(PutTask task) throws Exception { - HttpRequest.Builder request = - HttpRequest.newBuilder().uri(resolve(task)).timeout(Duration.ofMillis(requestTimeout)); + HttpRequest.Builder request = HttpRequest.newBuilder().uri(resolve(task)); if (expectContinue != null) { request = request.expectContinue(expectContinue); } @@ -423,8 +430,8 @@ protected void implClose() { } } - private static HttpClient createClient( - RepositorySystemSession session, RemoteRepository repository, boolean insecure) throws Exception { + private HttpClient createClient(RepositorySystemSession session, RemoteRepository repository, boolean insecure) + throws Exception { HashMap authentications = new HashMap<>(); SSLContext sslContext = null; @@ -474,12 +481,6 @@ public X509Certificate[] getAcceptedIssuers() { } } - int connectTimeout = ConfigUtils.getInteger( - session, - ConfigurationProperties.DEFAULT_CONNECT_TIMEOUT, - ConfigurationProperties.CONNECT_TIMEOUT + "." + repository.getId(), - ConfigurationProperties.CONNECT_TIMEOUT); - HttpClient.Builder builder = HttpClient.newBuilder() .version(HttpClient.Version.valueOf(ConfigUtils.getString( session, diff --git a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/test/java/org/eclipse/aether/transport/jdk/JdkTransporterTest.java b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/test/java/org/eclipse/aether/transport/jdk/JdkTransporterTest.java index 1a15aaed9..9bcd1395f 100644 --- a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/test/java/org/eclipse/aether/transport/jdk/JdkTransporterTest.java +++ b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/test/java/org/eclipse/aether/transport/jdk/JdkTransporterTest.java @@ -84,6 +84,11 @@ protected void testRetryHandler_explicitCount_positive() {} @Test protected void testPut_Authenticated_ExpectContinueRejected_ExplicitlyConfiguredHeader() {} + @Override + @Disabled + @Test + protected void testRequestTimeout() throws Exception {} + public JdkTransporterTest() { super(() -> new JdkTransporterFactory(standardChecksumExtractor(), new DefaultPathProcessor())); } diff --git a/maven-resolver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/JettyTransporter.java b/maven-resolver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/JettyTransporter.java index af78d285a..2935532b4 100644 --- a/maven-resolver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/JettyTransporter.java +++ b/maven-resolver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/JettyTransporter.java @@ -103,6 +103,8 @@ final class JettyTransporter extends AbstractTransporter implements HttpTranspor private final HttpClient client; + private final int connectTimeout; + private final int requestTimeout; private final Map headers; @@ -168,6 +170,11 @@ final class JettyTransporter extends AbstractTransporter implements HttpTranspor this.headers = headers; + this.connectTimeout = ConfigUtils.getInteger( + session, + ConfigurationProperties.DEFAULT_CONNECT_TIMEOUT, + ConfigurationProperties.CONNECT_TIMEOUT + "." + repository.getId(), + ConfigurationProperties.CONNECT_TIMEOUT); this.requestTimeout = ConfigUtils.getInteger( session, ConfigurationProperties.DEFAULT_REQUEST_TIMEOUT, @@ -228,9 +235,7 @@ public int classify(Throwable error) { @Override protected void implPeek(PeekTask task) throws Exception { - Request request = client.newRequest(resolve(task)) - .timeout(requestTimeout, TimeUnit.MILLISECONDS) - .method("HEAD"); + Request request = client.newRequest(resolve(task)).method("HEAD"); request.headers(m -> headers.forEach(m::add)); if (preemptiveAuth) { mayApplyPreemptiveAuth(request); @@ -248,9 +253,7 @@ protected void implGet(GetTask task) throws Exception { InputStreamResponseListener listener; while (true) { - Request request = client.newRequest(resolve(task)) - .timeout(requestTimeout, TimeUnit.MILLISECONDS) - .method("GET"); + Request request = client.newRequest(resolve(task)).method("GET"); request.headers(m -> headers.forEach(m::add)); if (preemptiveAuth) { mayApplyPreemptiveAuth(request); @@ -350,7 +353,7 @@ private static Function headerGetter(Response response) { @Override protected void implPut(PutTask task) throws Exception { - Request request = client.newRequest(resolve(task)).method("PUT").timeout(requestTimeout, TimeUnit.MILLISECONDS); + Request request = client.newRequest(resolve(task)).method("PUT"); request.headers(m -> headers.forEach(m::add)); if (preemptiveAuth || preemptivePutAuth) { mayApplyPreemptiveAuth(request); @@ -457,12 +460,6 @@ public X509Certificate[] getAcceptedIssuers() { } } - int connectTimeout = ConfigUtils.getInteger( - session, - ConfigurationProperties.DEFAULT_CONNECT_TIMEOUT, - ConfigurationProperties.CONNECT_TIMEOUT + "." + repository.getId(), - ConfigurationProperties.CONNECT_TIMEOUT); - SslContextFactory.Client sslContextFactory = new SslContextFactory.Client(); sslContextFactory.setSslContext(sslContext); if (insecure) { @@ -487,6 +484,7 @@ public X509Certificate[] getAcceptedIssuers() { HttpClient httpClient = new HttpClient(transport); httpClient.setConnectTimeout(connectTimeout); + httpClient.setIdleTimeout(requestTimeout); httpClient.setFollowRedirects(ConfigUtils.getBoolean( session, JettyTransporterConfigurationKeys.DEFAULT_FOLLOW_REDIRECTS, From ae6b42401314f7f66427e033f71d89f12cb37b5e Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Wed, 23 Oct 2024 15:28:56 +0200 Subject: [PATCH 2/3] Undo change by mistake --- .../org/eclipse/aether/transport/apache/ApacheTransporter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java b/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java index 73d881d56..e89f97519 100644 --- a/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java +++ b/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java @@ -299,7 +299,7 @@ final class ApacheTransporter extends AbstractTransporter implements HttpTranspo // the time to establish the connection (high level) .setConnectTimeout(connectTimeout) // the time to wait for a connection from the connection manager/pool - .setConnectionRequestTimeout(requestTimeout) + .setConnectionRequestTimeout(connectTimeout) .setLocalAddress(getHttpLocalAddress(session, repository)) .setCookieSpec(CookieSpecs.STANDARD) .build(); From 9233b24e6226a31163b58ed78a8245224a0b777b Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Wed, 23 Oct 2024 22:20:02 +0200 Subject: [PATCH 3/3] Undo change --- .../org/eclipse/aether/transport/apache/ApacheTransporter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java b/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java index e89f97519..36ac4a5c9 100644 --- a/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java +++ b/maven-resolver-transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java @@ -289,7 +289,7 @@ final class ApacheTransporter extends AbstractTransporter implements HttpTranspo .build(); SocketConfig socketConfig = // the time to establish connection (low level) - SocketConfig.custom().setSoTimeout(connectTimeout).build(); + SocketConfig.custom().setSoTimeout(requestTimeout).build(); RequestConfig requestConfig = RequestConfig.custom() .setMaxRedirects(maxRedirects) .setRedirectsEnabled(followRedirects)