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.

Sign up for GitHub

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

[MRESOLVER-612] Align timeout interpretations across HTTP transports #587

Merged
merged 5 commits into from
Oct 24, 2024
Merged

[MRESOLVER-612] Align timeout interpretations across HTTP transports #587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b87e928
[MRESOLVER-612] Align timeout interpretations across HTTP transports
cstamas Oct 23, 2024
ae6b424
Undo change by mistake
cstamas Oct 23, 2024
9233b24
Undo change
cstamas Oct 23, 2024
37fb398
Merge remote-tracking branch 'upstream/master' into MRESOLVER-612-bis
cstamas Oct 24, 2024
17c40bc
Merge remote-tracking branch 'upstream/master' into MRESOLVER-612-bis
cstamas Oct 24, 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
2 changes: 1 addition & 1 deletion maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion ...transport-apache/src/main/java/org/eclipse/aether/transport/apache/ApacheTransporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
37 changes: 19 additions & 18 deletions ...olver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@

/**
* 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
*/
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<Void> response = send(request.build(), HttpResponse.BodyHandlers.discarding());
Expand All @@ -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) {
Expand Down Expand Up @@ -385,8 +393,7 @@ private void closeBody(HttpResponse<InputStream> 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);
}
Expand Down Expand Up @@ -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<Authenticator.RequestorType, PasswordAuthentication> authentications = new HashMap<>();
SSLContext sslContext = null;
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions ...r-transport-jdk-11/src/test/java/org/eclipse/aether/transport/jdk/JdkTransporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Expand Down
24 changes: 11 additions & 13 deletions ...er-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/JettyTransporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -350,7 +353,7 @@ private static Function<String, String> 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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down