Merge pull request #1572 from presidentbeef/parallel_file_read_and_parse

Read and parse files in parallel

gem_common.rb

@@ -8,6 +8,7 @@ def self.dev_dependencies spec
     end
 
     def self.base_dependencies spec
+      spec.add_dependency "parallel", "~>1.20"
       spec.add_dependency "ruby_parser", "~>3.13"
       spec.add_dependency "ruby_parser-legacy", "~>1.0"
       spec.add_dependency "sexp_processor", "~> 4.7"

lib/brakeman/file_parser.rb

@@ -1,3 +1,5 @@
+require 'parallel'
+
 module Brakeman
   ASTFile = Struct.new(:path, :ast)
 
@@ -13,21 +15,46 @@ def initialize app_tree, timeout
     end
 
     def parse_files list
-      read_files list do |path, contents|
-        if ast = parse_ruby(contents, path.relative)
-          ASTFile.new(path, ast)
+      # Parse the files in parallel.
+      # By default, the parsing will be in separate processes.
+      # So we map the result to ASTFiles and/or Exceptions
+      # then partition them into ASTFiles and Exceptions
+      # and add the Exceptions to @errors
+      #
+      # Basically just a funky way to deal with two possible
+      # return types that are returned from isolated processes.
+      #
+      # Note this method no longer uses read_files
+      @file_list, new_errors = Parallel.map(list) do |file_name|
+        file_path = @app_tree.file_path(file_name)
+        contents = file_path.read
+
+        begin
+          if ast = parse_ruby(contents, file_path.relative)
+            ASTFile.new(file_name, ast)
+          end
+        rescue Exception => e
+          e
         end
+      end.compact.partition do |result|
+        result.is_a? ASTFile
       end
+
+      errors.concat new_errors
     end
 
     def read_files list
       list.each do |path|
         file = @app_tree.file_path(path)
 
-        result = yield file, file.read
+        begin
+          result = yield file, file.read
 
-        if result
-          @file_list << result
+          if result
+            @file_list << result
+          end
+        rescue Exception => e
+          @errors << e
         end
       end
     end
@@ -42,17 +69,12 @@ def parse_ruby input, path
         Brakeman.debug "Parsing #{path}"
         RubyParser.new.parse input, path, @timeout
       rescue Racc::ParseError => e
-        error e.exception(e.message + "\nCould not parse #{path}")
+        raise e.exception(e.message + "\nCould not parse #{path}")
       rescue Timeout::Error => e
-        error Exception.new("Parsing #{path} took too long (> #{@timeout} seconds). Try increasing the limit with --parser-timeout")
+        raise Exception.new("Parsing #{path} took too long (> #{@timeout} seconds). Try increasing the limit with --parser-timeout")
       rescue => e
-        error e.exception(e.message + "\nWhile processing #{path}")
+        raise e.exception(e.message + "\nWhile processing #{path}")
       end
     end
-
-    def error exception
-      @errors << exception
-      nil
-    end
   end
 end

lib/brakeman/scanner.rb

@@ -353,6 +353,9 @@ def index_call_sites
   def parse_ruby_file file
     fp = Brakeman::FileParser.new(tracker.app_tree, tracker.options[:parser_timeout])
     fp.parse_ruby(file.read, file)
+  rescue Exception => e
+    tracker.error(e)
+    nil
   end
 end
 

test/tests/file_parser.rb

@@ -1,4 +1,5 @@
 require_relative '../test'
+require 'tempfile'
 
 class FileParserTests < Minitest::Test
   def setup
@@ -8,21 +9,27 @@ def setup
   end
 
   def test_parse_error
-    @file_parser.parse_ruby <<-RUBY, "/tmp/BRAKEMAN_FAKE_PATH/test.rb"
-        x =
-    RUBY
+    tempfile = Tempfile.new
+    tempfile.write("x = ")
+    tempfile.close
 
+    @file_parser.parse_files([tempfile.path])
     @tracker.add_errors(@file_parser.errors)
 
     assert_equal 1, @tracker.errors.length
+  ensure
+    tempfile.unlink
   end
 
   def test_parse_error_shows_newer_failure
-    @file_parser.parse_ruby <<-RUBY, "/tmp/BRAKEMAN_FAKE_PATH/test.rb"
+    tempfile = Tempfile.new
+    tempfile.write <<-RUBY
     blah(x: 1)
     thing do
     RUBY
+    tempfile.close
 
+    @file_parser.parse_files([tempfile.path])
     @tracker.add_errors(@file_parser.errors)
 
     assert_equal 1, @tracker.errors.length
@@ -34,6 +41,20 @@ def test_parse_error_shows_newer_failure
     end
   end
 
+  def test_read_files_reports_error
+    tempfile = Tempfile.new
+    tempfile.write("x = ")
+    tempfile.close
+
+    @file_parser.read_files([tempfile.path]) do |path, contents|
+      @file_parser.parse_ruby contents, path
+    end
+
+    @tracker.add_errors(@file_parser.errors)
+
+    assert_equal 1, @tracker.errors.length
+  end
+
   def test_parse_ruby_accepts_file_path
     file_path = Brakeman::FilePath.from_app_tree @tracker.app_tree, "config/test.rb"