Skip to content
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.

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

ensuring connection timeouts get wrapped by MavenDownloadingException #4081

Merged
merged 8 commits into from
Mar 8, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public MavenPomDownloader(Map<Path, Pom> projectPoms, HttpSender httpSender, Exe
this.mavenCache = this.ctx.getPomCache();
}

byte[] sendRequest(HttpSender.Request request) throws Throwable {
byte[] sendRequest(HttpSender.Request request) throws IOException, HttpSenderResponseException {
long start = System.nanoTime();
try {
return Failsafe.with(retryPolicy).get(() -> {
Expand All @@ -148,7 +148,12 @@ byte[] sendRequest(HttpSender.Request request) throws Throwable {
}
});
} catch (FailsafeException failsafeException) {
throw failsafeException.getCause() == null ? failsafeException : failsafeException.getCause();
if (failsafeException.getCause() instanceof HttpSenderResponseException) {
throw (HttpSenderResponseException) failsafeException.getCause();
}
throw failsafeException;
} catch (UncheckedIOException e) {
throw e.getCause();
} finally {
this.ctx.recordResolutionTime(Duration.ofNanos(System.nanoTime() - start));
}
Expand Down Expand Up @@ -282,7 +287,7 @@ public MavenMetadata downloadMetadata(GroupArtifactVersion gav, @Nullable Resolv
.increment();
result = Optional.of(derivedMeta);
}
} catch (HttpSenderResponseException | MavenDownloadingException e) {
} catch (HttpSenderResponseException | MavenDownloadingException | IOException e) {
repositoryResponses.put(repo, e.getMessage());
}
}
Expand Down Expand Up @@ -328,7 +333,7 @@ public MavenMetadata downloadMetadata(GroupArtifactVersion gav, @Nullable Resolv
* @return Metadata or null if the metadata cannot be derived.
*/
@Nullable
private MavenMetadata deriveMetadata(GroupArtifactVersion gav, MavenRepository repo) throws HttpSenderResponseException, MavenDownloadingException {
private MavenMetadata deriveMetadata(GroupArtifactVersion gav, MavenRepository repo) throws HttpSenderResponseException, IOException, MavenDownloadingException {
if ((repo.getDeriveMetadataIfMissing() != null && !repo.getDeriveMetadataIfMissing()) || gav.getVersion() != null) {
// Do not derive metadata if we cannot navigate/browse the artifacts.
// Do not derive metadata if a specific version has been defined.
Expand Down Expand Up @@ -590,6 +595,8 @@ public Pom download(GroupArtifactVersion gav,
//If the exception is a common, client-side exception, cache an empty result.
mavenCache.putPom(resolvedGav, null);
}
} catch (IOException e) {
repositoryResponses.put(repo, e.getMessage());
}
}
} else if (result.isPresent()) {
Expand Down Expand Up @@ -769,8 +776,6 @@ public MavenRepository normalizeRepository(MavenRepository originalRepository, M
repository.getSnapshots(),
repository.getUsername(),
repository.getPassword());
} else if (!e.isClientSideException()) {
return originalRepository;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removing this feels a little cryptic, but I'm pretty sure it's no longer relevant?
eg, when it was written, the e.isServerReached call a few lines earlier did not exist:
3bcafba#diff-a39c7415cc78264a78509aaaba13d76b4eeea28cdc5723e15615b7a8add5b325

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that said, tests pass -- ready for review @timtebeek

}
} catch (Throwable e) {
// ok to fall through here and cache a null
Expand All @@ -797,7 +802,7 @@ public MavenRepository normalizeRepository(MavenRepository originalRepository, M
/**
* Replicates Apache Maven's behavior to attempt anonymous download if repository credentials prove invalid
*/
private byte[] requestAsAuthenticatedOrAnonymous(MavenRepository repo, String uriString) throws HttpSenderResponseException {
private byte[] requestAsAuthenticatedOrAnonymous(MavenRepository repo, String uriString) throws HttpSenderResponseException, IOException {
try {
return sendRequest(applyAuthenticationToRequest(repo, httpSender.get(uriString)).build());
} catch (HttpSenderResponseException e) {
Expand All @@ -806,12 +811,10 @@ private byte[] requestAsAuthenticatedOrAnonymous(MavenRepository repo, String ur
} else {
throw e;
}
} catch (Throwable t) {
throw new RuntimeException(t); // unreachable
}
}

private byte[] retryRequestAnonymously(String uriString, HttpSenderResponseException originalException) throws HttpSenderResponseException {
private byte[] retryRequestAnonymously(String uriString, HttpSenderResponseException originalException) throws HttpSenderResponseException, IOException {
try {
return sendRequest(httpSender.get(uriString).build());
} catch (HttpSenderResponseException retryException) {
Expand All @@ -820,8 +823,6 @@ private byte[] retryRequestAnonymously(String uriString, HttpSenderResponseExcep
} else {
throw retryException;
}
} catch (Throwable t) {
throw new RuntimeException(t); // unreachable
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.net.UnknownHostException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand All @@ -48,7 +49,6 @@
import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
Expand Down Expand Up @@ -86,7 +86,7 @@ void ossSonatype() {
null, "true", false, null, null, null);
MavenRepository repo = new MavenPomDownloader(ctx).normalizeRepository(ossSonatype,
MavenExecutionContextView.view(ctx), null);
assertThat(requireNonNull(repo).getUri()).isEqualTo(ossSonatype.getUri());
assertThat(repo).isNotNull().extracting((MavenRepository::getUri)).isEqualTo(ossSonatype.getUri());
}

@Issue("https://github.com/openrewrite/rewrite/issues/3908")
Expand Down Expand Up @@ -188,7 +188,7 @@ public void repositoryAccessFailed(String uri, Throwable e) {
// not expected to succeed
}
assertThat(attemptedUris).isNotEmpty();
assertThat(attemptedUris.get(httpUrl)).hasMessageContaining("java.net.UnknownHostException");
assertThat(attemptedUris.get(httpUrl)).isInstanceOf(UnknownHostException.class);
assertThat(discoveredRepositories).isEmpty();
}

Expand Down Expand Up @@ -742,6 +742,20 @@ void doNotRenameRepoForCustomMavenLocal(@TempDir Path tempDir) throws MavenDownl
assertThat(result.getRepository().getUri()).startsWith(tempDir.toUri().toString());
}

@Issue("https://github.com/openrewrite/rewrite/issues/4080")
@Test
void connectTimeout() {
var downloader = new MavenPomDownloader(ctx);
var gav = new GroupArtifactVersion("org.openrewrite", "rewrite-core", "7.0.0");
var repos = singletonList(MavenRepository.builder()
.id("non-routable").uri("http://10.0.0.0/maven").knownToExist(true).build());

assertThatThrownBy(() -> downloader.download(gav, null, null, repos))
.isInstanceOf(MavenDownloadingException.class)
.hasMessageContaining("rewrite-core")
.hasMessageContaining("10.0.0.0");
}

private static GroupArtifactVersion createArtifact(Path repository) throws IOException {
Path target = repository.resolve(Paths.get("org", "openrewrite", "rewrite", "1.0.0"));
Path pom = target.resolve("rewrite-1.0.0.pom");
Expand Down