Merge pull request #594 from stefanneuhaus/parallelize-analyzers-aftermath

Parallelize analyzers aftermath

dependency-check-core/src/main/java/org/owasp/dependencycheck/AnalysisTask.java

@@ -30,6 +30,7 @@ import java.util.concurrent.Callable;
 
 /**
  * Task to support parallelism of dependency-check analysis.
+ * Analyses a single {@link Dependency} by a specific {@link Analyzer}.
  *
  * @author Stefan Neuhaus
  */
@@ -80,7 +81,7 @@ class AnalysisTask implements Callable<Void> {
      * @throws Exception thrown if unable to execute the analysis task
      */
     @Override
-    public Void call() throws Exception {
+    public Void call() {
         Settings.initialize();
 
         if (shouldAnalyze()) {
@@ -107,10 +108,10 @@ class AnalysisTask implements Callable<Void> {
      *
      * @return whether or not the analyzer can analyze the dependency
      */
-    private boolean shouldAnalyze() {
+    boolean shouldAnalyze() {
         if (analyzer instanceof FileTypeAnalyzer) {
-            final FileTypeAnalyzer fAnalyzer = (FileTypeAnalyzer) analyzer;
+            final FileTypeAnalyzer fileTypeAnalyzer = (FileTypeAnalyzer) analyzer;
-            return fAnalyzer.accept(dependency.getActualFile());
+            return fileTypeAnalyzer.accept(dependency.getActualFile());
         }
 
         return true;

dependency-check-core/src/main/java/org/owasp/dependencycheck/Engine.java

@@ -47,6 +47,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.CancellationException;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -350,7 +351,7 @@ public class Engine implements FileFilter {
      * iterates over a copy of the dependencies list. Thus, the potential for
      * {@link java.util.ConcurrentModificationException}s is avoided, and
      * analyzers may safely add or remove entries from the dependencies list.
-     *
+     * <p>
      * Every effort is made to complete analysis on the dependencies. In some
      * cases an exception will occur with part of the analysis being performed
      * which may not affect the entire analysis. If an exception occurs it will
@@ -396,20 +397,20 @@ public class Engine implements FileFilter {
         for (AnalysisPhase phase : AnalysisPhase.values()) {
             final List<Analyzer> analyzerList = analyzers.get(phase);
 
-            for (final Analyzer a : analyzerList) {
+            for (final Analyzer analyzer : analyzerList) {
                 final long analyzerStart = System.currentTimeMillis();
                 try {
-                    initializeAnalyzer(a);
+                    initializeAnalyzer(analyzer);
                 } catch (InitializationException ex) {
                     exceptions.add(ex);
                     continue;
                 }
 
-                executeAnalysisTasks(exceptions, a);
+                executeAnalysisTasks(analyzer, exceptions);
 
                 final long analyzerDurationMillis = System.currentTimeMillis() - analyzerStart;
                 final long analyzerDurationSeconds = TimeUnit.MILLISECONDS.toSeconds(analyzerDurationMillis);
-                LOGGER.info("Finished {}. Took {} secs.", a.getName(), analyzerDurationSeconds);
+                LOGGER.info("Finished {}. Took {} secs.", analyzer.getName(), analyzerDurationSeconds);
             }
         }
         for (AnalysisPhase phase : AnalysisPhase.values()) {
@@ -429,11 +430,12 @@ public class Engine implements FileFilter {
 
     /**
      * Executes executes the analyzer using multiple threads.
+     *
      * @param exceptions a collection of exceptions that occurred during analysis
      * @param analyzer the analyzer to execute
      * @throws ExceptionCollection thrown if exceptions occurred during analysis
      */
-    private void executeAnalysisTasks(List<Throwable> exceptions, Analyzer analyzer) throws ExceptionCollection {
+    void executeAnalysisTasks(Analyzer analyzer, List<Throwable> exceptions) throws ExceptionCollection {
         LOGGER.debug("Starting {}", analyzer.getName());
         final List<AnalysisTask> analysisTasks = getAnalysisTasks(analyzer, exceptions);
         final ExecutorService executorService = getExecutorService(analyzer);
@@ -447,20 +449,25 @@ public class Engine implements FileFilter {
                     result.get();
                 } catch (ExecutionException e) {
                     throwFatalExceptionCollection("Analysis task failed with a fatal exception.", e, exceptions);
+                } catch (CancellationException e) {
+                    throwFatalExceptionCollection("Analysis task timed out.", e, exceptions);
                 }
             }
         } catch (InterruptedException e) {
             throwFatalExceptionCollection("Analysis has been interrupted.", e, exceptions);
+        } finally {
+            executorService.shutdown();
         }
     }
 
     /**
      * Returns the analysis tasks for the dependencies.
+     *
      * @param analyzer the analyzer to create tasks for
      * @param exceptions the collection of exceptions to collect
      * @return a collection of analysis tasks
      */
-    private List<AnalysisTask> getAnalysisTasks(Analyzer analyzer, List<Throwable> exceptions) {
+    List<AnalysisTask> getAnalysisTasks(Analyzer analyzer, List<Throwable> exceptions) {
         final List<AnalysisTask> result = new ArrayList<AnalysisTask>();
         synchronized (dependencies) {
             for (final Dependency dependency : dependencies) {
@@ -473,10 +480,11 @@ public class Engine implements FileFilter {
 
     /**
      * Returns the executor service for a given analyzer.
+     *
      * @param analyzer the analyzer to obtain an executor
      * @return the executor service
      */
-    private ExecutorService getExecutorService(Analyzer analyzer) {
+    ExecutorService getExecutorService(Analyzer analyzer) {
         if (analyzer.supportsParallelProcessing()) {
             // just a fair trade-off that should be reasonable for all analyzer types
             final int maximumNumberOfThreads = 4 * Runtime.getRuntime().availableProcessors();

dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/DependencyBundlingAnalyzer.java

@@ -58,10 +58,11 @@ public class DependencyBundlingAnalyzer extends AbstractAnalyzer {
      * A pattern for obtaining the first part of a filename.
      */
     private static final Pattern STARTING_TEXT_PATTERN = Pattern.compile("^[a-zA-Z0-9]*");
+
     /**
      * a flag indicating if this analyzer has run. This analyzer only runs once.
      */
-    private boolean analyzed = false;
+    boolean analyzed = false;
     //</editor-fold>
     //<editor-fold defaultstate="collapsed" desc="All standard implementation details of Analyzer">
     /**
@@ -95,7 +96,7 @@ public class DependencyBundlingAnalyzer extends AbstractAnalyzer {
     //</editor-fold>
 
     /**
-     * Does not support parallel processing as it both modifies and iterates over the engine's list of dependencies.
+     * Does not support parallel processing as it only runs once and then operates on <em>all</em> dependencies.
      *
      * @see #analyze(Dependency, Engine)
      */

dependency-check-core/src/test/java/org/owasp/dependencycheck/AnalysisTaskTest.java

@@ -0,0 +1,100 @@
+package org.owasp.dependencycheck;
+
+import mockit.Expectations;
+import mockit.Mocked;
+import mockit.Verifications;
+import org.junit.Test;
+import org.owasp.dependencycheck.analyzer.FileTypeAnalyzer;
+import org.owasp.dependencycheck.analyzer.HintAnalyzer;
+import org.owasp.dependencycheck.dependency.Dependency;
+
+import java.io.File;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class AnalysisTaskTest {
+
+    @Mocked
+    FileTypeAnalyzer fileTypeAnalyzer;
+
+    @Mocked
+    Dependency dependency;
+
+    @Mocked
+    Engine engine;
+
+
+    @Test
+    public void shouldAnalyzeReturnsTrueForNonFileTypeAnalyzers() {
+        AnalysisTask instance = new AnalysisTask(new HintAnalyzer(), null, null, null);
+        boolean shouldAnalyze = instance.shouldAnalyze();
+        assertTrue(shouldAnalyze);
+    }
+
+    @Test
+    public void shouldAnalyzeReturnsTrueIfTheFileTypeAnalyzersAcceptsTheDependency() {
+        final File dependencyFile = new File("");
+        new Expectations() {{
+            dependency.getActualFile();
+            result = dependencyFile;
+
+            fileTypeAnalyzer.accept(dependencyFile);
+            result = true;
+        }};
+
+        AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, null, null);
+
+        boolean shouldAnalyze = analysisTask.shouldAnalyze();
+        assertTrue(shouldAnalyze);
+    }
+
+    @Test
+    public void shouldAnalyzeReturnsFalseIfTheFileTypeAnalyzerDoesNotAcceptTheDependency() {
+        final File dependencyFile = new File("");
+        new Expectations() {{
+            dependency.getActualFile();
+            result = dependencyFile;
+
+            fileTypeAnalyzer.accept(dependencyFile);
+            result = false;
+        }};
+
+        AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, null, null);
+
+        boolean shouldAnalyze = analysisTask.shouldAnalyze();
+        assertFalse(shouldAnalyze);
+    }
+
+    @Test
+    public void taskAnalyzes() throws Exception {
+        final AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, engine, null);
+        new Expectations(analysisTask) {{
+            analysisTask.shouldAnalyze();
+            result = true;
+        }};
+
+        analysisTask.call();
+
+        new Verifications() {{
+            fileTypeAnalyzer.analyze(dependency, engine);
+            times = 1;
+        }};
+    }
+
+    @Test
+    public void taskDoesNothingIfItShouldNotAnalyze() throws Exception {
+        final AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, engine, null);
+        new Expectations(analysisTask) {{
+            analysisTask.shouldAnalyze();
+            result = false;
+        }};
+
+        analysisTask.call();
+
+        new Verifications() {{
+            fileTypeAnalyzer.analyze(dependency, engine);
+            times = 0;
+        }};
+    }
+}

dependency-check-core/src/test/java/org/owasp/dependencycheck/EngineTest.java

@@ -17,19 +17,36 @@
  */
 package org.owasp.dependencycheck;
 
-import java.io.File;
+import mockit.Expectations;
+import mockit.Mocked;
 import org.junit.Test;
-import static org.junit.Assert.*;
+import org.owasp.dependencycheck.analyzer.Analyzer;
 import org.owasp.dependencycheck.analyzer.JarAnalyzer;
 import org.owasp.dependencycheck.data.nvdcve.DatabaseException;
 import org.owasp.dependencycheck.dependency.Dependency;
+import org.owasp.dependencycheck.exception.ExceptionCollection;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 /**
- *
  * @author Jeremy Long
  */
 public class EngineTest extends BaseDBTestCase {
 
+    @Mocked
+    Analyzer analyzer;
+
+    @Mocked
+    AnalysisTask analysisTask;
+
+
     /**
      * Test of scanFile method, of class Engine.
      */
@@ -40,14 +57,40 @@ public class EngineTest extends BaseDBTestCase {
         File file = BaseTest.getResourceAsFile(this, "dwr.jar");
         Dependency dwr = instance.scanFile(file);
         file = BaseTest.getResourceAsFile(this, "org.mortbay.jmx.jar");
-        Dependency jmx = instance.scanFile(file);
+        instance.scanFile(file);
         assertEquals(2, instance.getDependencies().size());
-        
+
         file = BaseTest.getResourceAsFile(this, "dwr.jar");
         Dependency secondDwr = instance.scanFile(file);
-        
+
         assertEquals(2, instance.getDependencies().size());
         assertTrue(dwr == secondDwr);
-        
+    }
+
+    @Test(expected = ExceptionCollection.class)
+    public void exceptionDuringAnalysisTaskExecutionIsFatal() throws DatabaseException, ExceptionCollection {
+        final ExecutorService executorService = Executors.newFixedThreadPool(3);
+        final Engine instance = new Engine();
+        final List<Throwable> exceptions = new ArrayList<Throwable>();
+
+        new Expectations() {{
+            analysisTask.call();
+            result = new IllegalStateException("Analysis task execution threw an exception");
+        }};
+
+        final List<AnalysisTask> failingAnalysisTask = new ArrayList<AnalysisTask>();
+        failingAnalysisTask.add(analysisTask);
+
+        new Expectations(instance) {{
+            instance.getExecutorService(analyzer);
+            result = executorService;
+
+            instance.getAnalysisTasks(analyzer, exceptions);
+            result = failingAnalysisTask;
+        }};
+
+        instance.executeAnalysisTasks(analyzer, exceptions);
+
+        assertTrue(executorService.isShutdown());
     }
 }

dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CMakeAnalyzerTest.java

@@ -17,23 +17,31 @@
  */
 package org.owasp.dependencycheck.analyzer;
 
+import mockit.Mock;
+import mockit.MockUp;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.owasp.dependencycheck.BaseDBTestCase;
 import org.owasp.dependencycheck.BaseTest;
 import org.owasp.dependencycheck.Engine;
 import org.owasp.dependencycheck.analyzer.exception.AnalysisException;
 import org.owasp.dependencycheck.data.nvdcve.DatabaseException;
 import org.owasp.dependencycheck.dependency.Dependency;
+import org.owasp.dependencycheck.exception.InitializationException;
 
 import java.io.File;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.List;
 import java.util.regex.Pattern;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
-import org.owasp.dependencycheck.BaseDBTestCase;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Unit tests for CmakeAnalyzer.
@@ -150,4 +158,21 @@ public class CMakeAnalyzerTest extends BaseDBTestCase {
         assertTrue("Expected version evidence to contain \"" + version + "\".",
                 result.getVersionEvidence().toString().contains(version));
     }
+
+    @Test(expected = InitializationException.class)
+    public void analyzerIsDisabledInCaseOfMissingMessageDigest() throws InitializationException {
+        new MockUp<MessageDigest>() {
+            @Mock
+            MessageDigest getInstance(String ignore) throws NoSuchAlgorithmException {
+                throw new NoSuchAlgorithmException();
+            }
+        };
+
+        analyzer = new CMakeAnalyzer();
+        analyzer.setFilesMatched(true);
+        assertTrue(analyzer.isEnabled());
+        analyzer.initialize();
+
+        assertFalse(analyzer.isEnabled());
+    }
 }

dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/ComposerLockAnalyzerTest.java

@@ -17,19 +17,25 @@
  */
 package org.owasp.dependencycheck.analyzer;
 
+import mockit.Mock;
+import mockit.MockUp;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.owasp.dependencycheck.BaseDBTestCase;
 import org.owasp.dependencycheck.BaseTest;
 import org.owasp.dependencycheck.Engine;
 import org.owasp.dependencycheck.analyzer.exception.AnalysisException;
 import org.owasp.dependencycheck.dependency.Dependency;
+import org.owasp.dependencycheck.exception.InitializationException;
 
 import java.io.File;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import org.owasp.dependencycheck.BaseDBTestCase;
 
 /**
  * Unit tests for NodePackageAnalyzer.
@@ -96,4 +102,22 @@ public class ComposerLockAnalyzerTest extends BaseDBTestCase {
                 "composer.lock"));
         analyzer.analyze(result, engine);
     }
+
+
+    @Test(expected = InitializationException.class)
+    public void analyzerIsDisabledInCaseOfMissingMessageDigest() throws InitializationException {
+        new MockUp<MessageDigest>() {
+            @Mock
+            MessageDigest getInstance(String ignore) throws NoSuchAlgorithmException {
+                throw new NoSuchAlgorithmException();
+            }
+        };
+
+        analyzer = new ComposerLockAnalyzer();
+        analyzer.setFilesMatched(true);
+        assertTrue(analyzer.isEnabled());
+        analyzer.initialize();
+
+        assertFalse(analyzer.isEnabled());
+    }
 }

dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/DependencyBundlingAnalyzerTest.java

@@ -17,17 +17,25 @@
  */
 package org.owasp.dependencycheck.analyzer;
 
-import static org.junit.Assert.assertEquals;
+import mockit.Mocked;
+import mockit.Verifications;
 import org.junit.Test;
 import org.owasp.dependencycheck.BaseTest;
+import org.owasp.dependencycheck.Engine;
 import org.owasp.dependencycheck.dependency.Dependency;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
 /**
- *
  * @author Jeremy Long
  */
 public class DependencyBundlingAnalyzerTest extends BaseTest {
 
+    @Mocked
+    Engine engineMock;
+
     /**
      * Test of getName method, of class DependencyBundlingAnalyzer.
      */
@@ -52,15 +60,27 @@ public class DependencyBundlingAnalyzerTest extends BaseTest {
 
     /**
      * Test of analyze method, of class DependencyBundlingAnalyzer.
+     * The actually passed dependency does not matter. The analyzer only runs once.
      */
     @Test
     public void testAnalyze() throws Exception {
-//        Dependency ignore = null;
+        DependencyBundlingAnalyzer instance = new DependencyBundlingAnalyzer();
-//        Engine engine = null;
+
-//        DependencyBundlingAnalyzer instance = new DependencyBundlingAnalyzer();
+        // the actual dependency does not matter
-//        instance.analyze(ignore, engine);
+        assertFalse(instance.analyzed);
-//        // TODO review the generated test code and remove the default call to fail.
+        instance.analyze(null, engineMock);
-//        fail("The test case is a prototype.");
+
+        // the second runs basically does nothing
+        assertTrue(instance.analyzed);
+        instance.analyze(null, engineMock);
+        instance.analyze(null, engineMock);
+        instance.analyze(null, engineMock);
+        assertTrue(instance.analyzed);
+
+        new Verifications() {{
+            engineMock.getDependencies();
+            times = 2;
+        }};
     }
 
     /**
@@ -119,7 +139,5 @@ public class DependencyBundlingAnalyzerTest extends BaseTest {
         expResult = true;
         result = instance.firstPathIsShortest(left, right);
         assertEquals(expResult, result);
-
     }
-
 }