Update to Apache HttpComponents #130
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the HTTP client library from the deprecated Apache Commons HttpClient 3.1 to Apache HttpComponents 4.5.14. This modernization updates the HTTP request handling in the fetchURL method to use current, supported APIs with improved connection pooling and resource management patterns.
Key Changes:
- Updated dependency from
commons-httpclient:3.1toorg.apache.httpcomponents:httpclient:4.5.14andhttpcore:4.4.16 - Refactored HTTP client instantiation to use
CloseableHttpClientwithRequestConfigfor timeout/redirect settings - Migrated proxy configuration and authentication to use new HttpComponents 4.x APIs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pom.xml | Replaced deprecated commons-httpclient dependency with Apache HttpComponents httpclient 4.5.14 and httpcore 4.4.16 |
| src/main/java/org/apache/maven/plugin/doap/DoapUtil.java | Migrated fetchURL method from Commons HttpClient 3.x to HttpComponents 4.x APIs, including updated imports, client configuration, proxy setup, and response handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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(); |
There was a problem hiding this comment.
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.
| // 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(); |
There was a problem hiding this comment.
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.
| if (response != null) { | ||
| response.close(); | ||
| } | ||
| response = httpClient.execute(httpGet); |
There was a problem hiding this comment.
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.
| response = httpClient.execute(httpGet); | |
| // Create a new HttpGet instance for the retry attempt | |
| HttpGet retryHttpGet = new HttpGet(url.toString()); | |
| response = httpClient.execute(retryHttpGet); |
| try { | ||
| response.close(); | ||
| } catch (IOException e) { | ||
| // Log or ignore |
There was a problem hiding this comment.
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.
| // 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); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| * @see #DEFAULT_TIMEOUT | ||
| * @since 1.1 | ||
| */ | ||
| // TODO this method fetches a URL but the response seems to be ignored. |
There was a problem hiding this comment.
Looks like a checking only if given url exist .... so maybe method HEAD will be good enough here
There was a problem hiding this comment.
This checks if artefact is available in remote repo - so maybe check via resolver?
There was a problem hiding this comment.
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.
3 participants
I've updated the code, but I don't see what the fetchURL method actually does aside from pinging a URL with GET. The response seems to be ignored. Could we just delete it?