This is an automated email from the ASF dual-hosted git repository.
cstamas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven-resolver.git
The following commit(s) were added to refs/heads/master by this push:
new 0746435a [MRESOLVER-612][MRESOLVER-615] Align timeout interpretations
across HTTP transports (#587)
0746435a is described below
commit 0746435a68f40f49b89b6a84a29e157b27849825
Author: Tamas Cservenak <ta...@cservenak.net>
AuthorDate: Thu Oct 24 16:53:23 2024 +0200
[MRESOLVER-612][MRESOLVER-615] Align timeout interpretations across HTTP
transports (#587)
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.
Changes per transport:
* apache: explain meaning in comments and fix one mixed up timeout
* JDK: remove use of request timeout (as it means "abs max time for
request"), note that REQUEST_TIMEOUT is not supported, ignore UT
* Jetty: remove use of request timeout (as it means "abs max time for
request"), use "idle timeout" that means "max time of silence between any
traffic/sending or receiving packets".
---
https://issues.apache.org/jira/browse/MRESOLVER-612
https://issues.apache.org/jira/browse/MRESOLVER-615
---
.../eclipse/aether/ConfigurationProperties.java | 2 +-
.../aether/transport/apache/ApacheTransporter.java | 6 +++-
.../aether/transport/jdk/JdkTransporter.java | 37 +++++++++++-----------
.../aether/transport/jdk/JdkTransporterTest.java | 5 +++
.../aether/transport/jetty/JettyTransporter.java | 24 +++++++-------
5 files changed, 41 insertions(+), 33 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 0179733a..9ee3ccf1 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 cb54e467..36ac4a5c 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 =
+ // the time to establish connection (low level)
SocketConfig.custom().setSoTimeout(requestTimeout).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)
+ // the time to wait for a connection from the connection
manager/pool
.setConnectionRequestTimeout(connectTimeout)
.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 1f835c0e..b3cbf8b4 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 @@ import static
org.eclipse.aether.transport.jdk.JdkTransporterConfigurationKeys.D
/**
* JDK Transport using {@link HttpClient}.
+ * <p>
+ * Known issues:
+ * <ul>
+ * <li>Does not support {@link ConfigurationProperties#REQUEST_TIMEOUT},
see <a href="https://bugs.openjdk.org/browse/JDK-8258397";>JDK-8258397</a></li>
+ * </ul>
*
* @since 2.0.0
*/
@@ -122,6 +127,8 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
private final Map<String, String> 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 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
@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<Void> response = send(request.build(),
HttpResponse.BodyHandlers.discarding());
@@ -266,10 +276,8 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
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 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
@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 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
}
}
- private static HttpClient createClient(
- RepositorySystemSession session, RemoteRepository repository,
boolean insecure) throws Exception {
+ private HttpClient createClient(RepositorySystemSession session,
RemoteRepository repository, boolean insecure)
+ throws Exception {
HashMap<Authenticator.RequestorType, PasswordAuthentication>
authentications = new HashMap<>();
SSLContext sslContext = null;
@@ -474,12 +481,6 @@ final class JdkTransporter extends AbstractTransporter
implements HttpTransporte
}
}
- 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 1a15aaed..9bcd1395 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 @@ class JdkTransporterTest extends HttpTransporterTest {
@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 af78d285..2935532b 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<String, String> 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 @@ final class JettyTransporter extends AbstractTransporter
implements HttpTranspor
@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 @@ final class JettyTransporter extends AbstractTransporter
implements HttpTranspor
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 @@ final class JettyTransporter extends AbstractTransporter
implements HttpTranspor
@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 @@ final class JettyTransporter extends AbstractTransporter
implements HttpTranspor
}
}
- 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 @@ final class JettyTransporter extends AbstractTransporter
implements HttpTranspor
HttpClient httpClient = new HttpClient(transport);
httpClient.setConnectTimeout(connectTimeout);
+ httpClient.setIdleTimeout(requestTimeout);
httpClient.setFollowRedirects(ConfigUtils.getBoolean(
session,
JettyTransporterConfigurationKeys.DEFAULT_FOLLOW_REDIRECTS,
