Skip to content

Update to Apache HttpComponents #130

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

Closed
elharo wants to merge 5 commits into from
+76 −31
Closed

Update to Apache HttpComponents #130

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
c587991
Update to Apache HttpComponents
elharo Nov 29, 2025
2695fbe
comment
elharo Nov 29, 2025
f5758d2
comment
elharo Nov 29, 2025
9755fd1
comment
elharo Nov 29, 2025
430eb05
Update src/main/java/org/apache/maven/plugin/doap/DoapUtil.java
elharo Nov 29, 2025
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
11 changes: 8 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,14 @@ under the License.
<version>3.17.0</version>
</dependency>
<dependency>
<groupId>commons-httpclient</groupId>
<artifactId>commons-httpclient</artifactId>
<version>3.1</version>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<version>4.5.14</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpcore</artifactId>
<version>4.4.16</version>
</dependency>

<!-- test -->
Expand Down
96 changes: 68 additions & 28 deletions src/main/java/org/apache/maven/plugin/doap/DoapUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,19 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.apache.commons.httpclient.Credentials;
import org.apache.commons.httpclient.HttpClient;
import org.apache.commons.httpclient.HttpStatus;
import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager;
import org.apache.commons.httpclient.UsernamePasswordCredentials;
import org.apache.commons.httpclient.auth.AuthScope;
import org.apache.commons.httpclient.methods.GetMethod;
import org.apache.commons.httpclient.params.HttpClientParams;
import org.apache.commons.httpclient.params.HttpMethodParams;
import org.apache.http.HttpHost;
import org.apache.http.HttpStatus;
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.UsernamePasswordCredentials;
import org.apache.http.client.CredentialsProvider;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClients;
import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
import org.apache.http.util.EntityUtils;
import org.apache.jena.rdf.model.Model;
import org.apache.jena.rdf.model.ModelFactory;
import org.apache.jena.rdf.model.RDFReader;
Expand Down Expand Up @@ -465,6 +469,9 @@ public static boolean isValidEmail(String str) {
* @see #DEFAULT_TIMEOUT
* @since 1.1
*/
// TODO this method fetches a URL but the response seems to be ignored.
Copy link
Member

@slawekjaranowski slawekjaranowski Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a checking only if given url exist .... so maybe method HEAD will be good enough here

Copy link
Member

@slachiewicz slachiewicz Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks if artefact is available in remote repo - so maybe check via resolver?

Copy link
Contributor Author

@elharo elharo Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I'll try that approach in a day or two unless someone gets there first. I don't think we need to proceed further with this PR.

// Does it actually do anything? Is this effectively a NOOP?
// Is it testing reachability?
@SuppressWarnings("checkstyle:emptyblock")
public static void fetchURL(Settings settings, URL url) throws IOException {
if (url == null) {
Expand All @@ -479,17 +486,16 @@ public static void fetchURL(Settings settings, URL url) throws IOException {
return;
}

// http, https...
HttpClient httpClient = new HttpClient(new MultiThreadedHttpConnectionManager());
httpClient.getHttpConnectionManager().getParams().setConnectionTimeout(DEFAULT_TIMEOUT);
httpClient.getHttpConnectionManager().getParams().setSoTimeout(DEFAULT_TIMEOUT);
httpClient.getParams().setBooleanParameter(HttpClientParams.ALLOW_CIRCULAR_REDIRECTS, true);
// 1. Configure Request Parameters (Timeouts and Redirects)
RequestConfig.Builder requestConfigBuilder = RequestConfig.custom()
.setConnectTimeout(DEFAULT_TIMEOUT)
.setSocketTimeout(DEFAULT_TIMEOUT)
.setCircularRedirectsAllowed(true); // Replaces ALLOW_CIRCULAR_REDIRECTS

// Some web servers don't allow the default user-agent sent by httpClient
httpClient
.getParams()
.setParameter(HttpMethodParams.USER_AGENT, "Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)");
// 2. Prepare Credentials Provider
CredentialsProvider credentialsProvider = new BasicCredentialsProvider();

// 3. Configure Proxy
if (settings != null && settings.getActiveProxy() != null) {
Proxy activeProxy = settings.getActiveProxy();

Expand All @@ -498,32 +504,66 @@ public static void fetchURL(Settings settings, URL url) throws IOException {

if (StringUtils.isNotEmpty(activeProxy.getHost())
&& !ProxyUtils.validateNonProxyHosts(proxyInfo, url.getHost())) {
httpClient.getHostConfiguration().setProxy(activeProxy.getHost(), activeProxy.getPort());

if (StringUtils.isNotEmpty(activeProxy.getUsername()) && activeProxy.getPassword() != null) {
Credentials credentials =
new UsernamePasswordCredentials(activeProxy.getUsername(), activeProxy.getPassword());
// Set the proxy on the configuration
HttpHost proxy = new HttpHost(activeProxy.getHost(), activeProxy.getPort());
requestConfigBuilder.setProxy(proxy);

httpClient.getState().setProxyCredentials(AuthScope.ANY, credentials);
if (StringUtils.isNotEmpty(activeProxy.getUsername()) && activeProxy.getPassword() != null) {
credentialsProvider.setCredentials(
new AuthScope(proxy), // Scope to the specific proxy
new UsernamePasswordCredentials(activeProxy.getUsername(), activeProxy.getPassword()));
}
}
}

GetMethod getMethod = new GetMethod(url.toString());
// Ideally, the ConnectionManager and Client should be singletons reused across the application,
// not created per request.
CloseableHttpClient httpClient = HttpClients.custom()
.setConnectionManager(new PoolingHttpClientConnectionManager())
.setDefaultCredentialsProvider(credentialsProvider)
.setDefaultRequestConfig(requestConfigBuilder.build())
.setUserAgent("Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)")
.build();
Comment on lines +522 to +527
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CloseableHttpClient is never closed, causing a resource leak. The client should be closed in the finally block to properly release the connection manager and its resources. Consider using a try-with-resources statement or explicitly closing the client in the finally block after closing the response.

Copilot uses AI. Check for mistakes.
Comment on lines +520 to +527
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new PoolingHttpClientConnectionManager and CloseableHttpClient for each request is inefficient and defeats the purpose of connection pooling. As the comment acknowledges, these should be created once and reused across all requests. Consider refactoring to use a singleton pattern or static instance for better performance and resource management.

Copilot uses AI. Check for mistakes.

HttpGet httpGet = new HttpGet(url.toString());
CloseableHttpResponse response = null;

try {
int status;
try {
status = httpClient.executeMethod(getMethod);
response = httpClient.execute(httpGet);
status = response.getStatusLine().getStatusCode();
} catch (SocketTimeoutException e) {
// could be a sporadic failure, one more retry before we give up
status = httpClient.executeMethod(getMethod);
// Retry logic: Close previous response if it exists (unlikely on timeout) and retry once
if (response != null) {
response.close();
}
response = httpClient.execute(httpGet);
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On SocketTimeoutException, the retry logic attempts to reuse the same HttpGet object. However, after a failed request attempt, the request object may be in an inconsistent state. It's safer to create a new HttpGet instance for the retry attempt.

Suggested change
response = httpClient.execute(httpGet);
// Create a new HttpGet instance for the retry attempt
HttpGet retryHttpGet = new HttpGet(url.toString());
response = httpClient.execute(retryHttpGet);

Copilot uses AI. Check for mistakes.
status = response.getStatusLine().getStatusCode();
}

if (status != HttpStatus.SC_OK) {
// Ensure entity is consumed before throwing exception to release connection
if (response.getEntity() != null) {
EntityUtils.consume(response.getEntity());
}
throw new FileNotFoundException(url.toString());
} else {
// Ensure entity is consumed in the success case to release connection for reuse
if (response.getEntity() != null) {
EntityUtils.consume(response.getEntity());
}
}
} finally {
getMethod.releaseConnection();
// In 4.5, closing the response releases the connection back to the pool
if (response != null) {
try {
response.close();
} catch (IOException e) {
// Log or ignore
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silently ignoring the IOException when closing the response makes debugging difficult. Consider logging the exception or at minimum adding a comment explaining why it's safe to ignore. If a logger is not available in this context, document why ignoring this exception is acceptable.

Suggested change
// Log or ignore
// Log the exception to aid debugging
System.err.println("Warning: IOException occurred while closing HTTP response: " + e.getMessage());
e.printStackTrace(System.err);

Copilot uses AI. Check for mistakes.
}
}
}
}

Expand Down