Copilot commented on code in PR #130:
URL: https://github.com/apache/maven-doap-plugin/pull/130#discussion_r2573067900
##########
src/main/java/org/apache/maven/plugin/doap/DoapUtil.java:
##########
@@ -498,32 +504,61 @@ 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();
Review 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.
##########
src/main/java/org/apache/maven/plugin/doap/DoapUtil.java:
##########
@@ -498,32 +504,61 @@ 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();
Review 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.
##########
src/main/java/org/apache/maven/plugin/doap/DoapUtil.java:
##########
@@ -498,32 +504,61 @@ 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();
+
+ 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);
Review 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.
```suggestion
// Create a new HttpGet instance for the retry attempt
HttpGet retryHttpGet = new HttpGet(url.toString());
response = httpClient.execute(retryHttpGet);
```
##########
src/main/java/org/apache/maven/plugin/doap/DoapUtil.java:
##########
@@ -498,32 +504,61 @@ 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();
+
+ 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);
+ 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());
}
} 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
Review 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.
```suggestion
// Log the exception to aid debugging
System.err.println("Warning: IOException occurred while
closing HTTP response: " + e.getMessage());
e.printStackTrace(System.err);
```
##########
src/main/java/org/apache/maven/plugin/doap/DoapUtil.java:
##########
@@ -498,32 +504,61 @@ 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();
+
+ 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);
+ 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());
Review Comment:
When the status is `SC_OK` (success case), the response entity is never
consumed. While closing the response should release the connection, it's best
practice to consume the entity explicitly using
`EntityUtils.consume(response.getEntity())` to ensure proper connection reuse
and avoid potential connection pool starvation. Add entity consumption for the
success case, similar to the error case at line 549.
```suggestion
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());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
