From 3c3534e7da239bb21d72d4c5fa8626774266ad95 Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Mon, 2 Oct 2017 17:01:16 +0200 Subject: [PATCH] CentralAnalyzer: Implement retry for fetching MavenArtifacts due to sporadic issues --- .../analyzer/CentralAnalyzer.java | 58 +++++++- .../data/central/CentralSearch.java | 10 +- .../analyzer/CentralAnalyzerTest.java | 127 ++++++++++++++++++ 3 files changed, 187 insertions(+), 8 deletions(-) create mode 100644 dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java index 55d267832..638693d88 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java @@ -74,6 +74,13 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { */ private static final String SUPPORTED_EXTENSIONS = "jar"; + /** + * There may be temporary issues when connecting to MavenCentral. + * In order to compensate for 99% of the issues, we perform a retry + * before finally raising the {@link #errorFlag}. + */ + private static final int NUMBER_OF_TRIES = 5; + /** * The analyzer should be disabled if there are errors, so this is a flag to * determine if such an error has occurred. @@ -83,7 +90,8 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { /** * The searcher itself. */ - private CentralSearch searcher; + CentralSearch searcher; + /** * Field indicating if the analyzer is enabled. */ @@ -212,7 +220,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { } try { - final List mas = searcher.searchSha1(dependency.getSha1sum()); + final List mas = fetchMavenArtifacts(dependency); final Confidence confidence = mas.size() > 1 ? Confidence.HIGH : Confidence.HIGHEST; for (MavenArtifact ma : mas) { LOGGER.debug("Central analyzer found artifact ({}) for dependency ({})", ma, dependency.getFileName()); @@ -256,8 +264,52 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { } catch (FileNotFoundException fnfe) { LOGGER.debug("Artifact not found in repository: '{}", dependency.getFileName()); } catch (IOException ioe) { - LOGGER.debug("Could not connect to Central search", ioe); + LOGGER.warn("Could not connect to Central search. Disabling this analyzer.", ioe); errorFlag = true; } } + + /** + * Downloads the corresponding list of MavenArtifacts of the given + * dependency from MavenCentral. + *

+ * As the connection to MavenCentral is known to be unreliable, we implement + * a simple retry logic in order to compensate for 99% of the issues. + * + * @param dependency the dependency to analyze + * @return the downloaded list of MavenArtifacts + * @throws FileNotFoundException if the specified artifact is not found + * @throws IOException if connecting to MavenCentral finally failed + */ + List fetchMavenArtifacts(Dependency dependency) throws IOException { + IOException lastException = null; + long sleepingTimeBetweenRetriesInMillis = 1000; + int triesLeft = NUMBER_OF_TRIES; + while (triesLeft-- > 0) { + try { + return searcher.searchSha1(dependency.getSha1sum()); + } catch (FileNotFoundException fnfe) { + // retry does not make sense, just throw the exception + throw fnfe; + } catch (IOException ioe) { + LOGGER.debug("Could not connect to Central search (tries left: {}): {}", + triesLeft, ioe.getMessage()); + lastException = ioe; + + if (triesLeft > 0) { + try { + Thread.sleep(sleepingTimeBetweenRetriesInMillis); + sleepingTimeBetweenRetriesInMillis *= 2; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + } + } + + LOGGER.warn("Finally failed connecting to Central search." + + " Giving up after {} tries. Last exception was: {}", + NUMBER_OF_TRIES, lastException); + throw lastException; + } } diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java index 8466741ef..5c1d46557 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java @@ -101,8 +101,9 @@ public class CentralSearch { * * @param sha1 the SHA-1 hash string for which to search * @return the populated Maven GAV. - * @throws IOException if it's unable to connect to the specified repository - * or if the specified artifact is not found. + * @throws FileNotFoundException if the specified artifact is not found + * @throws IOException if it's unable to connect to the specified + * repository */ public List searchSha1(String sha1) throws IOException { if (null == sha1 || !sha1.matches("^[0-9A-Fa-f]{40}$")) { @@ -178,9 +179,8 @@ public class CentralSearch { throw new FileNotFoundException("Artifact not found in Central"); } } else { - LOGGER.debug("Could not connect to Central received response code: {} {}", - conn.getResponseCode(), conn.getResponseMessage()); - throw new IOException("Could not connect to Central"); + String errorMessage = "Could not connect to MavenCentral (" + conn.getResponseCode() + "): " + conn.getResponseMessage(); + throw new IOException(errorMessage); } return result; } diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java new file mode 100644 index 000000000..e7a129a06 --- /dev/null +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java @@ -0,0 +1,127 @@ +package org.owasp.dependencycheck.analyzer; + +import mockit.Expectations; +import mockit.Mock; +import mockit.MockUp; +import mockit.Mocked; +import org.junit.BeforeClass; +import org.junit.Test; +import org.owasp.dependencycheck.data.central.CentralSearch; +import org.owasp.dependencycheck.data.nexus.MavenArtifact; +import org.owasp.dependencycheck.dependency.Dependency; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Collections; +import java.util.List; + +import static org.junit.Assert.assertEquals; + +/** + * Tests for the CentralAnalyzer. + */ +public class CentralAnalyzerTest { + + private static final String SHA1_SUM = "my-sha1-sum"; + + @BeforeClass + public static void beforeClass() { + doNotSleepBetweenRetries(); + } + + @Test + public void testFetchMavenArtifactsWithoutException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + final List expectedMavenArtifacts = Collections.emptyList(); + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + returns(expectedMavenArtifacts); + }}; + + final List actualMavenArtifacts = instance.fetchMavenArtifacts(dependency); + + assertEquals(expectedMavenArtifacts, actualMavenArtifacts); + } + + @Test + public void testFetchMavenArtifactsWithSporadicIOException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + final List expectedMavenArtifacts = Collections.emptyList(); + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + result = new IOException("Could not connect to MavenCentral (500): Internal Server Error"); + result = new IOException("Could not connect to MavenCentral (500): Internal Server Error"); + result = expectedMavenArtifacts; + }}; + + final List actualMavenArtifacts = instance.fetchMavenArtifacts(dependency); + + assertEquals(expectedMavenArtifacts, actualMavenArtifacts); + } + + @Test(expected = FileNotFoundException.class) + public void testFetchMavenArtifactsRethrowsFileNotFoundException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + result = new FileNotFoundException("Artifact not found in Central"); + }}; + + instance.fetchMavenArtifacts(dependency); + } + + @Test(expected = IOException.class) + public void testFetchMavenArtifactsAlwaysThrowsIOException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + result = new IOException("no internet connection"); + }}; + + instance.fetchMavenArtifacts(dependency); + } + + /** + * We do not want to waste time in unit tests. + */ + private static void doNotSleepBetweenRetries() { + new MockUp() { + @Mock + void sleep(long millis) { + // do not sleep + } + }; + } +}