Skip to content

rescue less #55

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 18, 2015
Merged

rescue less #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86723c6
Be specific about what exceptions can be skipped
wfleming Dec 17, 2015
818ada3
refactor: extract CommandLineRunner
wfleming Dec 17, 2015
724e48f
Timeout errors should be fatal
wfleming Dec 17, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions lib/cc/engine/analyzers/analyzer_base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,26 @@
require "cc/engine/analyzers/parser_error"

module CC
module Engine
module Analyzers
class Base
RESCUABLE_ERRORS = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opinion question for the team: should we also catch Timeout::Error? Flay does internally (not in a code path we're using, FWIW). I think the thinking behind it was that if the parser took too long on a file, it's probably a parser bug (or maybe an absurdly large file). We were implicitly catching it before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd try to match the behavior of Flay run on the command line then. If the Flay catches the timeout error, logs a skip, and continues on, then I think that we should do the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dblandin 's reasoning sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree as well. It has been added now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm going to back this out: I remembered why I was hesitant about this in the first place. This series of changes was prompted by concerns of non-deterministic results from this engine (due to memory settings, but really a variety of system issues could come into play).

These timeouts are, by their nature, slightly non-deterministic. a file complex & big enough to be borderline might timeout some times & not others. Memory pressure on the system could cause execution time to vary between one run & another. Etc.

So instead I think I'm going to consider the timeout errors fatal & up the timeout limit to something absurd like 5 minutes. That should ensure it only gets triggered in pathological cases (like a parser bug leading to an infinite loop or something).

@jpignata this is relevant to your interests, so your thoughts are welcome.

::CC::Engine::Analyzers::ParserError,
::Racc::ParseError,
]

def initialize(engine_config:)
@engine_config = engine_config
end

def run(file)
process_file(file)
rescue *RESCUABLE_ERRORS => ex
$stderr.puts("Skipping file #{file} due to exception:")
$stderr.puts("(#{ex.class}) #{ex.message} #{ex.backtrace.join("\n")}")
rescue => ex
$stderr.puts "Skipping file #{file} due to exception"
$stderr.puts "(#{ex.class}) #{ex.message} #{ex.backtrace.join("\n")}"
$stderr.puts("#{ex.class} error occurred processing file #{file}: aborting.")
raise ex
end

def files
Expand Down
45 changes: 45 additions & 0 deletions lib/cc/engine/analyzers/command_line_runner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
require "open3"
require "timeout"

module CC
module Engine
module Analyzers
class CommandLineRunner
DEFAULT_TIMEOUT = 300

def initialize(command, timeout = DEFAULT_TIMEOUT)
@command = command
@timeout = timeout
end

def run(input)
Timeout.timeout(timeout) do
Open3.popen3 command, "r+" do |stdin, stdout, stderr, wait_thr|
stdin.puts input
stdin.close

exit_code = wait_thr.value

output = stdout.gets
stdout.close

err_output = stderr.gets
stderr.close

if 0 == exit_code
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that there was a exit_code.to_i before the move. Is that still needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Different invocation. This is already an int.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

yield output
else
raise ::CC::Engine::Analyzers::ParserError, "Python parser exited with code #{exit_code}:\n#{err_output}"
end
end
end
end

private

attr_reader :command, :timeout
end
end
end
end

34 changes: 2 additions & 32 deletions lib/cc/engine/analyzers/javascript/parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require 'timeout'
require "cc/engine/analyzers/command_line_runner"

module CC
module Engine
Expand All @@ -13,7 +13,7 @@ def initialize(code, filename)
end

def parse
runner = CommandLineRunner.new(js_command, self)
runner = CommandLineRunner.new(js_command)
runner.run(strip_shebang(code)) do |ast|
json_ast = JSON.parse(ast)
@syntax_tree = json_ast
Expand All @@ -37,36 +37,6 @@ def strip_shebang(code)
end
end
end

class CommandLineRunner
attr_reader :command, :delegate

DEFAULT_TIMEOUT = 20
EXCEPTIONS = [
StandardError,
Timeout::Error,
SystemStackError
]

def initialize(command, delegate)
@command = command
@delegate = delegate
end

def run(input, timeout = DEFAULT_TIMEOUT)
Timeout.timeout(timeout) do
IO.popen command, 'r+' do |io|
io.puts input
io.close_write

output = io.gets
io.close

yield output if $?.to_i == 0
end
end
end
end
end
end
end
Expand Down
7 changes: 7 additions & 0 deletions lib/cc/engine/analyzers/parser_error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module CC
module Engine
module Analyzers
ParserError = Class.new(StandardError)
end
end
end
29 changes: 3 additions & 26 deletions lib/cc/engine/analyzers/php/parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'cc/engine/analyzers/php/ast'
require 'cc/engine/analyzers/php/nodes'
require "cc/engine/analyzers/command_line_runner"
require "cc/engine/analyzers/php/ast"
require "cc/engine/analyzers/php/nodes"

module CC
module Engine
Expand Down Expand Up @@ -36,30 +37,6 @@ def parser_path
)
end
end

class CommandLineRunner
attr_reader :command, :delegate

DEFAULT_TIMEOUT = 20

def initialize(command)
@command = command
end

def run(input, timeout = DEFAULT_TIMEOUT)
Timeout.timeout(timeout) do
IO.popen command, 'r+' do |io|
io.puts input
io.close_write

output = io.gets
io.close

yield output if $?.to_i == 0
end
end
end
end
end
end
end
Expand Down
32 changes: 4 additions & 28 deletions lib/cc/engine/analyzers/python/parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'timeout'
require 'json'
require "cc/engine/analyzers/command_line_runner"
require "timeout"
require "json"

module CC
module Engine
Expand All @@ -14,7 +15,7 @@ def initialize(code, filename)
end

def parse
runner = CommandLineRunner.new(python_command, self)
runner = CommandLineRunner.new(python_command)
runner.run(code) do |ast|
json_ast = JSON.parse(ast)
@syntax_tree = json_ast
Expand All @@ -28,31 +29,6 @@ def python_command
"python #{file}"
end
end

class CommandLineRunner
DEFAULT_TIMEOUT = 20

attr_reader :command, :delegate

def initialize(command, delegate)
@command = command
@delegate = delegate
end

def run(input, timeout = DEFAULT_TIMEOUT)
Timeout.timeout(timeout) do
IO.popen command, "r+" do |io|
io.puts input
io.close_write

output = io.gets
io.close

yield output if $?.to_i == 0
end
end
end
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/cc/engine/analyzers/ruby/main.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class Main < CC::Engine::Analyzers::Base
]
DEFAULT_MASS_THRESHOLD = 18
BASE_POINTS = 10_000
TIMEOUT = 10
TIMEOUT = 300

private

Expand Down
10 changes: 10 additions & 0 deletions spec/cc/engine/analyzers/javascript/main_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@
expect(json["content"]["body"]).to match /This issue has a mass of `99`/
expect(json["fingerprint"]).to eq("55ae5d0990647ef496e9e0d315f9727d")
end

it "skips unparsable files" do
create_source_file("foo.js", <<-EOJS)
function () { do(); // missing closing brace
EOJS

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
end
end

it "does not flag duplicate comments" do
Expand Down
10 changes: 10 additions & 0 deletions spec/cc/engine/analyzers/php/main_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@
expect(json["content"]["body"]).to match /This issue has a mass of `44`/
expect(json["fingerprint"]).to eq("667da0e2bab866aa2fe9d014a65d57d9")
end

it "skips unparsable files" do
create_source_file("foo.php", <<-EOPHP)
<?php blorb &; "fee
EOPHP

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
end
end

def printed_issue
Expand Down
10 changes: 10 additions & 0 deletions spec/cc/engine/analyzers/python/main_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@
expect(json["content"]["body"]).to match /This issue has a mass of `54`/
expect(json["fingerprint"]).to eq("42b832387c997f54a2012efb2159aefc")
end

it "skips unparsable files" do
create_source_file("foo.py", <<-EOPY)
---
EOPY

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
end
end

def engine_conf
Expand Down