gnodet-bot commented on code in PR #29781:
URL: https://github.com/apache/camel/pull/29781#discussion_r4556971798
##########
tooling/camel-tooling-maven/src/main/java/org/apache/camel/tooling/maven/MavenDownloaderImpl.java:
##########
@@ -840,9 +857,91 @@ private List<RemoteRepository> resolutionRepositories(
session, extraRepos);
repositories.addAll(resolvedExtraRepos);
}
+ if (!unreachableRepositories.isEmpty()) {
+ // asking them again would only wait for the connect timeout again
+ repositories.removeIf(r ->
unreachableRepositories.contains(repositoryUrl(r.getUrl())));
+ }
return repositories;
}
+ /**
+ * Sets the request timeout of Camel (unless configured, such as with the
aether.connector.requestTimeout system
+ * property), and stops using a repository that cannot be connected to.
+ */
+ private RepositorySystemSession
withTimeoutsAndReachability(RepositorySystemSession session) {
+ DefaultRepositorySystemSession answer = new
DefaultRepositorySystemSession(session);
+ if
(session.getConfigProperties().get(ConfigurationProperties.REQUEST_TIMEOUT) ==
null) {
+ answer.setConfigProperty(ConfigurationProperties.REQUEST_TIMEOUT,
requestTimeout);
+ }
+
answer.setTransferListener(ChainedTransferListener.newInstance(session.getTransferListener(),
+ new UnreachableRepositoryListener(unreachableRepositories)));
+ return answer;
+ }
+
+ /**
+ * Notes the repositories that cannot be connected to: a host that does
not resolve, refuses the connection, or does
+ * not answer within the connect timeout. A repository that is slow to
send an artifact is still used.
+ */
+ static final class UnreachableRepositoryListener extends
AbstractTransferListener {
+
+ private final Set<String> unreachable;
+
+ UnreachableRepositoryListener(Set<String> unreachable) {
+ this.unreachable = unreachable;
+ }
+
+ @Override
+ public void transferFailed(TransferEvent event) {
+ Throwable cause = connectFailure(event.getException());
+ if (cause != null && event.getResource() != null &&
event.getResource().getRepositoryUrl() != null) {
+ String url =
repositoryUrl(event.getResource().getRepositoryUrl());
+ if (unreachable.add(url)) {
+ LOG.warn("Cannot connect to Maven repository: {} due to:
{}. The repository is not used anymore.",
+ url, cause.getMessage() != null ?
cause.getMessage() : cause.getClass().getName());
+ }
+ }
+ }
+
+ static Throwable connectFailure(Throwable e) {
+ // guard against a cause that points back to an exception in the
chain
+ int depth = 0;
+ for (Throwable t = e; t != null && depth < 20; t = t.getCause(),
depth++) {
+ if (t instanceof ConnectException || t instanceof
UnknownHostException) {
+ return t;
+ }
+ // the connect timeout of the JDK, and of Apache HttpClient
(ConnectTimeoutException)
+ if (t instanceof SocketTimeoutException && "Connect timed
out".equalsIgnoreCase(t.getMessage())
+ ||
"org.apache.http.conn.ConnectTimeoutException".equals(t.getClass().getName())) {
Review Comment:
💡 **Nit (optional):** `&&` already binds tighter than `||` in Java so the
logic is correct, but explicit parentheses on the second condition make the
precedence unmistakable for the next reader:
```suggestion
if (t instanceof SocketTimeoutException && "Connect timed
out".equalsIgnoreCase(t.getMessage())
||
("org.apache.http.conn.ConnectTimeoutException".equals(t.getClass().getName())))
{
```
##########
tooling/camel-tooling-maven/src/test/java/org/apache/camel/tooling/maven/MavenDownloaderImplTest.java:
##########
@@ -1130,4 +1138,94 @@ private static void writePom(File repo, String
artifactId, String body) throws E
""".formatted(artifactId, body);
Files.writeString(new File(dir, artifactId + "-1.0.0.pom").toPath(),
pom);
}
+
+ @Test
+ void testUnreachableRepositoryIsNotUsedAnymore() throws Exception {
+ File repo = createDescriptorFixtureRepository();
+ File localRepo = new File(tempDir, "unreachable-local-repo");
+ // nothing listens on this port, so the connection is refused
+ String dead = "http://localhost:"; +
AvailablePortFinder.getNextAvailable(20000, 30000) + "/maven";
+
+ try (MavenDownloaderImpl downloader = new MavenDownloaderImpl()) {
+ downloader.setMavenCentralEnabled(false);
+ downloader.setMavenApacheSnapshotEnabled(false);
+ downloader.setMavenSettingsLocation("false");
+ downloader.setRepos(dead + "," + repo.toURI());
+ downloader.build();
+ MavenDownloader customDownloader =
downloader.customize(localRepo.getAbsolutePath(), 5000, 5000);
+
+ // an artifact no repository has: both are asked
+ assertThrows(MavenResolutionException.class,
+ () ->
customDownloader.resolveArtifacts(List.of("org.example.fixture:missing:1.0.0"),
null, false,
+ false));
+ assertEquals(Set.of(dead),
downloader.getUnreachableRepositories());
+
+ // the other repository is still used
+ List<MavenArtifact> artifacts = customDownloader.resolveArtifacts(
+ List.of("org.example.fixture:fixture-bom:pom:1.0.0"),
null, false, false);
+ assertEquals(1, artifacts.size());
+
+ // the repository is not asked again, and the failure says so
+ MavenResolutionException e =
assertThrows(MavenResolutionException.class,
+ () ->
customDownloader.resolveArtifacts(List.of("org.example.fixture:missing2:1.0.0"),
null, false,
+ false));
+ assertTrue(e.getRepositories().contains(dead + " (not
reachable)"), e.getRepositories().toString());
+ assertFalse(e.getRepositories().contains(dead),
e.getRepositories().toString());
+ }
+ }
+
+ @Test
+ void testRepositoryThatDoesNotAnswerFailsOnTheRequestTimeout() throws
Exception {
+ File localRepo = new File(tempDir, "stalled-local-repo");
+ // accepts the connection, and never answers
+ try (ServerSocket server = new ServerSocket(0);
+ MavenDownloaderImpl downloader = new MavenDownloaderImpl()) {
+ List<Socket> accepted = new CopyOnWriteArrayList<>();
+ Thread acceptor = new Thread(() -> {
+ try {
+ while (!server.isClosed()) {
+ accepted.add(server.accept());
+ }
+ } catch (Exception e) {
+ // closed
+ }
+ });
+ acceptor.setDaemon(true);
+ acceptor.start();
+ String stalled = "http://localhost:"; + server.getLocalPort() +
"/maven";
+
+ downloader.setMavenCentralEnabled(false);
+ downloader.setMavenApacheSnapshotEnabled(false);
+ downloader.setMavenSettingsLocation("false");
+ downloader.setRepos(stalled);
+ downloader.build();
+ assertEquals(60000, downloader.getRequestTimeout());
+ MavenDownloader customDownloader =
downloader.customize(localRepo.getAbsolutePath(), 5000, 1000);
+
+ long start = System.nanoTime();
+ assertThrows(MavenResolutionException.class,
+ () ->
customDownloader.resolveArtifacts(List.of("org.example.fixture:stalled:1.0.0"),
null, false,
+ false));
+ assertTrue(TimeUnit.NANOSECONDS.toSeconds(System.nanoTime() -
start) < 30);
+ // a slow repository is still used
+ assertTrue(downloader.getUnreachableRepositories().isEmpty());
+ for (Socket socket : accepted) {
+ socket.close();
+ }
+ }
+ }
+
+ @Test
+ void testConnectFailures() {
+
assertNotNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(
+ new RuntimeException(new ConnectException("Connection
refused"))));
+
assertNotNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(new
UnknownHostException("nope")));
+
assertNotNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(
+ new SocketTimeoutException("Connect timed out")));
+ // a read timeout is a slow repository, not an unreachable one
+
assertNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(
+ new SocketTimeoutException("Read timed out")));
+
assertNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(new
IOException("Not found")));
+
assertNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(null));
+ }
Review Comment:
⚠️ **Missing coverage: `org.apache.http.conn.ConnectTimeoutException` branch
untested**
`connectFailure()` checks for `org.apache.http.conn.ConnectTimeoutException`
by class name (to avoid a compile-time dep on `httpclient`). However,
`httpclient` 4.5.14 is already a compile-scope transitive dependency (via
`maven-resolver-transport-http`), so this branch can and should be tested
directly. Without it, a typo in the class-name string literal silently breaks
connect-timeout detection for the HTTP transport.
```suggestion
void testConnectFailures() {
assertNotNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(
new RuntimeException(new ConnectException("Connection
refused"))));
assertNotNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(new
UnknownHostException("nope")));
assertNotNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(
new SocketTimeoutException("Connect timed out")));
// Apache HttpClient 4.x wraps connect timeout in its own exception
type (not a plain SocketTimeoutException)
assertNotNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(
new org.apache.http.conn.ConnectTimeoutException("Connect to
localhost:1234 timed out")));
// a read timeout is a slow repository, not an unreachable one
assertNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(
new SocketTimeoutException("Read timed out")));
assertNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(new
IOException("Not found")));
assertNull(MavenDownloaderImpl.UnreachableRepositoryListener.connectFailure(null));
}
```
##########
tooling/camel-tooling-maven/src/main/java/org/apache/camel/tooling/maven/MavenDownloaderImpl.java:
##########
@@ -840,9 +857,91 @@ private List<RemoteRepository> resolutionRepositories(
session, extraRepos);
repositories.addAll(resolvedExtraRepos);
}
+ if (!unreachableRepositories.isEmpty()) {
+ // asking them again would only wait for the connect timeout again
+ repositories.removeIf(r ->
unreachableRepositories.contains(repositoryUrl(r.getUrl())));
+ }
return repositories;
}
+ /**
+ * Sets the request timeout of Camel (unless configured, such as with the
aether.connector.requestTimeout system
+ * property), and stops using a repository that cannot be connected to.
+ */
+ private RepositorySystemSession
withTimeoutsAndReachability(RepositorySystemSession session) {
+ DefaultRepositorySystemSession answer = new
DefaultRepositorySystemSession(session);
+ if
(session.getConfigProperties().get(ConfigurationProperties.REQUEST_TIMEOUT) ==
null) {
+ answer.setConfigProperty(ConfigurationProperties.REQUEST_TIMEOUT,
requestTimeout);
+ }
+
answer.setTransferListener(ChainedTransferListener.newInstance(session.getTransferListener(),
+ new UnreachableRepositoryListener(unreachableRepositories)));
+ return answer;
+ }
+
+ /**
+ * Notes the repositories that cannot be connected to: a host that does
not resolve, refuses the connection, or does
+ * not answer within the connect timeout. A repository that is slow to
send an artifact is still used.
+ */
+ static final class UnreachableRepositoryListener extends
AbstractTransferListener {
+
+ private final Set<String> unreachable;
+
+ UnreachableRepositoryListener(Set<String> unreachable) {
+ this.unreachable = unreachable;
+ }
+
+ @Override
+ public void transferFailed(TransferEvent event) {
+ Throwable cause = connectFailure(event.getException());
+ if (cause != null && event.getResource() != null &&
event.getResource().getRepositoryUrl() != null) {
+ String url =
repositoryUrl(event.getResource().getRepositoryUrl());
+ if (unreachable.add(url)) {
+ LOG.warn("Cannot connect to Maven repository: {} due to:
{}. The repository is not used anymore.",
+ url, cause.getMessage() != null ?
cause.getMessage() : cause.getClass().getName());
+ }
+ }
+ }
+
+ static Throwable connectFailure(Throwable e) {
+ // guard against a cause that points back to an exception in the
chain
+ int depth = 0;
+ for (Throwable t = e; t != null && depth < 20; t = t.getCause(),
depth++) {
+ if (t instanceof ConnectException || t instanceof
UnknownHostException) {
+ return t;
+ }
+ // the connect timeout of the JDK, and of Apache HttpClient
(ConnectTimeoutException)
+ if (t instanceof SocketTimeoutException && "Connect timed
out".equalsIgnoreCase(t.getMessage())
+ ||
"org.apache.http.conn.ConnectTimeoutException".equals(t.getClass().getName())) {
+ return t;
Review Comment:
ℹ️ **Forward-compatibility note: HttpClient 5 transport**
This class-name check covers HttpClient 4.x
(`org.apache.http.conn.ConnectTimeoutException`). When maven-resolver upgrades
to its HttpClient 5 transport, the class moves to
`org.apache.hc.client5.http.ConnectTimeoutException` — which extends
`SocketTimeoutException`, but its message includes host details (e.g. `"Connect
to localhost:1234 timed out"`) rather than the exact string `"Connect timed
out"`. Neither branch would match.
Consider adding a comment documenting this forward-compat concern so the
next upgrader knows to update both the class-name check and the test.
--
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]
