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 78d8eb62 [MRESOLVER-584] JDK transport HTTP/2 GOAWAY improvement (#532) 78d8eb62 is described below commit 78d8eb623efb9bbf918ae007eb05ce20dab8c816 Author: Tamas Cservenak <ta...@cservenak.net> AuthorDate: Fri Aug 2 15:19:33 2024 +0200 [MRESOLVER-584] JDK transport HTTP/2 GOAWAY improvement (#532) Changes: * drop 21 closer, it abruptly uses "shutdown now" * in 11 make httpClient NOT stored per session, but per transport instance * simplify and refactor, put things in places like insecure mode config --- https://issues.apache.org/jira/browse/MRESOLVER-584 --- .../aether/transport/jdk/JdkTransporter.java | 275 +++++++++------------ .../aether/transport/jdk/JdkTransporterCloser.java | 2 +- .../maven-resolver-transport-jdk-21/pom.xml | 98 -------- .../aether/transport/jdk/JdkTransporterCloser.java | 32 --- .../maven-resolver-transport-jdk/pom.xml | 18 -- maven-resolver-transport-jdk-parent/pom.xml | 1 - 6 files changed, 122 insertions(+), 304 deletions(-) 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 32da764e..15a37326 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 @@ -197,6 +197,17 @@ final class JdkTransporter extends AbstractTransporter implements HttpTransporte javaVersion); } } + final String httpsSecurityMode = ConfigUtils.getString( + session, + ConfigurationProperties.HTTPS_SECURITY_MODE_DEFAULT, + ConfigurationProperties.HTTPS_SECURITY_MODE + "." + repository.getId(), + ConfigurationProperties.HTTPS_SECURITY_MODE); + + if (!ConfigurationProperties.HTTPS_SECURITY_MODE_DEFAULT.equals(httpsSecurityMode) + && !ConfigurationProperties.HTTPS_SECURITY_MODE_INSECURE.equals(httpsSecurityMode)) { + throw new IllegalArgumentException("Unsupported '" + httpsSecurityMode + "' HTTPS security mode."); + } + final boolean insecure = ConfigurationProperties.HTTPS_SECURITY_MODE_INSECURE.equals(httpsSecurityMode); this.maxConcurrentRequests = new Semaphore(ConfigUtils.getInteger( session, @@ -205,7 +216,11 @@ final class JdkTransporter extends AbstractTransporter implements HttpTransporte CONFIG_PROP_MAX_CONCURRENT_REQUESTS)); this.headers = headers; - this.client = getOrCreateClient(session, repository, javaVersion); + try { + this.client = createClient(session, repository, insecure); + } catch (Exception e) { + throw new NoTransporterException(repository, e); + } } private URI resolve(TransportTask task) { @@ -386,7 +401,7 @@ final class JdkTransporter extends AbstractTransporter implements HttpTransporte } private <T> HttpResponse<T> send(HttpRequest request, HttpResponse.BodyHandler<T> responseBodyHandler) - throws IOException, InterruptedException { + throws Exception { maxConcurrentRequests.acquire(); try { return client.send(request, responseBodyHandler); @@ -397,176 +412,134 @@ final class JdkTransporter extends AbstractTransporter implements HttpTransporte @Override protected void implClose() { - // no-op - } - - private InetAddress getHttpLocalAddress(RepositorySystemSession session, RemoteRepository repository) { - String bindAddress = ConfigUtils.getString( - session, - null, - ConfigurationProperties.HTTP_LOCAL_ADDRESS + "." + repository.getId(), - ConfigurationProperties.HTTP_LOCAL_ADDRESS); - if (bindAddress == null) { - return null; - } - try { - return InetAddress.getByName(bindAddress); - } catch (UnknownHostException uhe) { - throw new IllegalArgumentException( - "Given bind address (" + bindAddress + ") cannot be resolved for remote repository " + repository, - uhe); + if (client != null) { + JdkTransporterCloser.closer(client).run(); } } - /** - * Visible for testing. - */ - static final String HTTP_INSTANCE_KEY_PREFIX = JdkTransporterFactory.class.getName() + ".http."; + private static HttpClient createClient( + RepositorySystemSession session, RemoteRepository repository, boolean insecure) throws Exception { - private HttpClient getOrCreateClient(RepositorySystemSession session, RemoteRepository repository, int javaVersion) - throws NoTransporterException { - final String instanceKey = HTTP_INSTANCE_KEY_PREFIX + repository.getId(); + HashMap<Authenticator.RequestorType, PasswordAuthentication> authentications = new HashMap<>(); + SSLContext sslContext = null; + try (AuthenticationContext repoAuthContext = AuthenticationContext.forRepository(session, repository)) { + if (repoAuthContext != null) { + sslContext = repoAuthContext.get(AuthenticationContext.SSL_CONTEXT, SSLContext.class); - final String httpsSecurityMode = ConfigUtils.getString( - session, - ConfigurationProperties.HTTPS_SECURITY_MODE_DEFAULT, - ConfigurationProperties.HTTPS_SECURITY_MODE + "." + repository.getId(), - ConfigurationProperties.HTTPS_SECURITY_MODE); + String username = repoAuthContext.get(AuthenticationContext.USERNAME); + String password = repoAuthContext.get(AuthenticationContext.PASSWORD); - if (!ConfigurationProperties.HTTPS_SECURITY_MODE_DEFAULT.equals(httpsSecurityMode) - && !ConfigurationProperties.HTTPS_SECURITY_MODE_INSECURE.equals(httpsSecurityMode)) { - throw new IllegalArgumentException("Unsupported '" + httpsSecurityMode + "' HTTPS security mode."); + authentications.put( + Authenticator.RequestorType.SERVER, + new PasswordAuthentication(username, password.toCharArray())); + } } - final boolean insecure = ConfigurationProperties.HTTPS_SECURITY_MODE_INSECURE.equals(httpsSecurityMode); - // todo: normally a single client per JVM is sufficient - in particular cause part of the config - // is global and not per instance so we should create a client only when conf changes for a repo - // else fallback on a global client - try { - return (HttpClient) session.getData().computeIfAbsent(instanceKey, () -> { - HashMap<Authenticator.RequestorType, PasswordAuthentication> authentications = new HashMap<>(); - SSLContext sslContext = null; - try { - try (AuthenticationContext repoAuthContext = - AuthenticationContext.forRepository(session, repository)) { - if (repoAuthContext != null) { - sslContext = repoAuthContext.get(AuthenticationContext.SSL_CONTEXT, SSLContext.class); + if (sslContext == null) { + if (insecure) { + sslContext = SSLContext.getInstance("TLS"); + X509ExtendedTrustManager tm = new X509ExtendedTrustManager() { + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType) {} - String username = repoAuthContext.get(AuthenticationContext.USERNAME); - String password = repoAuthContext.get(AuthenticationContext.PASSWORD); + @Override + public void checkServerTrusted(X509Certificate[] chain, String authType) {} - authentications.put( - Authenticator.RequestorType.SERVER, - new PasswordAuthentication(username, password.toCharArray())); - } - } + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket) {} - if (sslContext == null) { - if (insecure) { - sslContext = SSLContext.getInstance("TLS"); - X509ExtendedTrustManager tm = new X509ExtendedTrustManager() { - @Override - public void checkClientTrusted(X509Certificate[] chain, String authType) {} - - @Override - public void checkServerTrusted(X509Certificate[] chain, String authType) {} - - @Override - public void checkClientTrusted( - X509Certificate[] chain, String authType, Socket socket) {} - - @Override - public void checkServerTrusted( - X509Certificate[] chain, String authType, Socket socket) {} - - @Override - public void checkClientTrusted( - X509Certificate[] chain, String authType, SSLEngine engine) {} - - @Override - public void checkServerTrusted( - X509Certificate[] chain, String authType, SSLEngine engine) {} - - @Override - public X509Certificate[] getAcceptedIssuers() { - return null; - } - }; - sslContext.init(null, new X509TrustManager[] {tm}, null); - } else { - sslContext = SSLContext.getDefault(); - } - } + @Override + public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket) {} - 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, - DEFAULT_HTTP_VERSION, - CONFIG_PROP_HTTP_VERSION + "." + repository.getId(), - CONFIG_PROP_HTTP_VERSION))) - .followRedirects(HttpClient.Redirect.NORMAL) - .connectTimeout(Duration.ofMillis(connectTimeout)) - .sslContext(sslContext); - - if (insecure) { - SSLParameters sslParameters = sslContext.getDefaultSSLParameters(); - sslParameters.setEndpointIdentificationAlgorithm(null); - builder.sslParameters(sslParameters); - } + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine) {} - setLocalAddress(builder, () -> getHttpLocalAddress(session, repository)); + @Override + public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine) {} - if (repository.getProxy() != null) { - ProxySelector proxy = ProxySelector.of(new InetSocketAddress( - repository.getProxy().getHost(), - repository.getProxy().getPort())); + @Override + public X509Certificate[] getAcceptedIssuers() { + return null; + } + }; + sslContext.init(null, new X509TrustManager[] {tm}, null); + } else { + sslContext = SSLContext.getDefault(); + } + } - builder.proxy(proxy); - try (AuthenticationContext proxyAuthContext = - AuthenticationContext.forProxy(session, repository)) { - if (proxyAuthContext != null) { - String username = proxyAuthContext.get(AuthenticationContext.USERNAME); - String password = proxyAuthContext.get(AuthenticationContext.PASSWORD); + 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, + DEFAULT_HTTP_VERSION, + CONFIG_PROP_HTTP_VERSION + "." + repository.getId(), + CONFIG_PROP_HTTP_VERSION))) + .followRedirects(HttpClient.Redirect.NORMAL) + .connectTimeout(Duration.ofMillis(connectTimeout)) + .sslContext(sslContext); + + if (insecure) { + SSLParameters sslParameters = sslContext.getDefaultSSLParameters(); + sslParameters.setEndpointIdentificationAlgorithm(null); + builder.sslParameters(sslParameters); + } - authentications.put( - Authenticator.RequestorType.PROXY, - new PasswordAuthentication(username, password.toCharArray())); - } - } - } + setLocalAddress(builder, () -> getHttpLocalAddress(session, repository)); - if (!authentications.isEmpty()) { - builder.authenticator(new Authenticator() { - @Override - protected PasswordAuthentication getPasswordAuthentication() { - return authentications.get(getRequestorType()); - } - }); - } + if (repository.getProxy() != null) { + ProxySelector proxy = ProxySelector.of(new InetSocketAddress( + repository.getProxy().getHost(), repository.getProxy().getPort())); - HttpClient result = builder.build(); - if (!session.addOnSessionEndedHandler(JdkTransporterCloser.closer(javaVersion, result))) { - LOGGER.warn( - "Using Resolver 2 feature without Resolver 2 session handling, you may leak resources."); - } + builder.proxy(proxy); + try (AuthenticationContext proxyAuthContext = AuthenticationContext.forProxy(session, repository)) { + if (proxyAuthContext != null) { + String username = proxyAuthContext.get(AuthenticationContext.USERNAME); + String password = proxyAuthContext.get(AuthenticationContext.PASSWORD); - return result; - } catch (Exception e) { - throw new WrapperEx(e); + authentications.put( + Authenticator.RequestorType.PROXY, + new PasswordAuthentication(username, password.toCharArray())); + } + } + } + + if (!authentications.isEmpty()) { + builder.authenticator(new Authenticator() { + @Override + protected PasswordAuthentication getPasswordAuthentication() { + return authentications.get(getRequestorType()); } }); - } catch (WrapperEx e) { - throw new NoTransporterException(repository, e.getCause()); } + + return builder.build(); } - private void setLocalAddress(HttpClient.Builder builder, Supplier<InetAddress> addressSupplier) { + private static InetAddress getHttpLocalAddress(RepositorySystemSession session, RemoteRepository repository) { + String bindAddress = ConfigUtils.getString( + session, + null, + ConfigurationProperties.HTTP_LOCAL_ADDRESS + "." + repository.getId(), + ConfigurationProperties.HTTP_LOCAL_ADDRESS); + if (bindAddress == null) { + return null; + } + try { + return InetAddress.getByName(bindAddress); + } catch (UnknownHostException uhe) { + throw new IllegalArgumentException( + "Given bind address (" + bindAddress + ") cannot be resolved for remote repository " + repository, + uhe); + } + } + + private static void setLocalAddress(HttpClient.Builder builder, Supplier<InetAddress> addressSupplier) { try { final InetAddress address = addressSupplier.get(); if (address == null) { @@ -586,10 +559,4 @@ final class JdkTransporter extends AbstractTransporter implements HttpTransporte throw new IllegalStateException(e); } } - - private static final class WrapperEx extends RuntimeException { - private WrapperEx(Throwable cause) { - super(cause); - } - } } diff --git a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporterCloser.java b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporterCloser.java index a273c014..69f839a7 100644 --- a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporterCloser.java +++ b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-11/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporterCloser.java @@ -27,7 +27,7 @@ import java.net.http.HttpClient; */ public final class JdkTransporterCloser { @SuppressWarnings("checkstyle:MagicNumber") - static Runnable closer(int javaVersion, HttpClient httpClient) { + static Runnable closer(HttpClient httpClient) { return () -> { if (httpClient instanceof AutoCloseable) { try { diff --git a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-21/pom.xml b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-21/pom.xml deleted file mode 100644 index cdf0e305..00000000 --- a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-21/pom.xml +++ /dev/null @@ -1,98 +0,0 @@ -<?xml version="1.0" encoding="UTF-8"?> -<!-- - Licensed to the Apache Software Foundation (ASF) under one - or more contributor license agreements. See the NOTICE file - distributed with this work for additional information - regarding copyright ownership. The ASF licenses this file - to you under the Apache License, Version 2.0 (the - "License"); you may not use this file except in compliance - with the License. You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, - software distributed under the License is distributed on an - "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - KIND, either express or implied. See the License for the - specific language governing permissions and limitations - under the License. ---> -<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> - <modelVersion>4.0.0</modelVersion> - - <parent> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-transport-jdk-parent</artifactId> - <version>2.0.1-SNAPSHOT</version> - </parent> - - <artifactId>maven-resolver-transport-jdk-21</artifactId> - <packaging>jar</packaging> - - <name>Maven Artifact Resolver Transport JDK 21</name> - <description>Maven Artifact Transport JDK Java 11+.</description> - - <properties> - <javaVersion>21</javaVersion> - </properties> - - <dependencies> - <dependency> - <groupId>org.slf4j</groupId> - <artifactId>slf4j-api</artifactId> - </dependency> - <dependency> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-api</artifactId> - </dependency> - <dependency> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-spi</artifactId> - </dependency> - <dependency> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-util</artifactId> - </dependency> - <dependency> - <groupId>javax.inject</groupId> - <artifactId>javax.inject</artifactId> - <scope>provided</scope> - <optional>true</optional> - </dependency> - - <dependency> - <groupId>org.junit.jupiter</groupId> - <artifactId>junit-jupiter-api</artifactId> - <scope>test</scope> - </dependency> - <dependency> - <groupId>org.slf4j</groupId> - <artifactId>slf4j-simple</artifactId> - <scope>test</scope> - </dependency> - <dependency> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-test-util</artifactId> - <scope>test</scope> - </dependency> - <dependency> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-test-http</artifactId> - <scope>test</scope> - </dependency> - <dependency> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-impl</artifactId> - <scope>test</scope> - </dependency> - </dependencies> - - <build> - <plugins> - <plugin> - <groupId>org.eclipse.sisu</groupId> - <artifactId>sisu-maven-plugin</artifactId> - </plugin> - </plugins> - </build> -</project> diff --git a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-21/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporterCloser.java b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-21/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporterCloser.java deleted file mode 100644 index b7da9a81..00000000 --- a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk-21/src/main/java/org/eclipse/aether/transport/jdk/JdkTransporterCloser.java +++ /dev/null @@ -1,32 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.eclipse.aether.transport.jdk; - -import java.net.http.HttpClient; - -/** - * JDK Transport that properly closes {@link HttpClient} on Java 21+. - * - * @since 2.0.0 - */ -public final class JdkTransporterCloser { - static Runnable closer(int javaVersion, HttpClient httpClient) { - return httpClient::shutdownNow; - } -} diff --git a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk/pom.xml b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk/pom.xml index 7fe556a5..97f03208 100644 --- a/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk/pom.xml +++ b/maven-resolver-transport-jdk-parent/maven-resolver-transport-jdk/pom.xml @@ -53,12 +53,6 @@ <version>${project.version}</version> <optional>true</optional> </dependency> - <dependency> - <groupId>org.apache.maven.resolver</groupId> - <artifactId>maven-resolver-transport-jdk-21</artifactId> - <version>${project.version}</version> - <optional>true</optional> - </dependency> <dependency> <groupId>org.slf4j</groupId> @@ -135,18 +129,6 @@ <includes>**/*.class</includes> </configuration> </execution> - <execution> - <id>java21</id> - <goals> - <goal>unpack-dependencies</goal> - </goals> - <phase>generate-resources</phase> - <configuration> - <includeArtifactIds>maven-resolver-transport-jdk-21</includeArtifactIds> - <outputDirectory>${project.build.directory}/generated-resources/META-INF/versions/21</outputDirectory> - <includes>**/*.class</includes> - </configuration> - </execution> </executions> </plugin> </plugins> diff --git a/maven-resolver-transport-jdk-parent/pom.xml b/maven-resolver-transport-jdk-parent/pom.xml index 83d26deb..16d4966e 100644 --- a/maven-resolver-transport-jdk-parent/pom.xml +++ b/maven-resolver-transport-jdk-parent/pom.xml @@ -35,7 +35,6 @@ <modules> <module>maven-resolver-transport-jdk-8</module> <module>maven-resolver-transport-jdk-11</module> - <module>maven-resolver-transport-jdk-21</module> <module>maven-resolver-transport-jdk</module> </modules> </project>