Skip to content

Commit cdc50e0

Browse files
committed
Replace Open3 with POSIX::Spawn
Customers are experiencing capture3 returning a nil status in production. It results in an engine error output of: NoMethodError: undefined method `success?' for nil:NilClass block in run at /usr/src/app/lib/cc/engine/analyzers/command_line_runner.rb:18 The theory is: the Open3 code internally uses Process.detach to get the wait thread, which returns nil if the pid given to it is not a valid pid. So my guess was that the parser ran & exited so quickly that by the time detach was called it didn't exist anymore. But it may very well have ran successfully. Moving to POSIX::Spawn, which we typically us in all code anyway, should behave more robustly in this sense.
1 parent dfdc557 commit cdc50e0

File tree

4 files changed

+43
-8
lines changed

4 files changed

+43
-8
lines changed

Gemfile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ gem 'ruby_parser'
66
gem 'pry'
77
gem 'sexp_processor'
88
gem 'json'
9+
gem 'posix-spawn'
910

1011
group :test do
1112
gem 'rake'

Gemfile.lock

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ GEM
1212
diff-lcs (1.2.5)
1313
json (1.8.3)
1414
method_source (0.8.2)
15+
posix-spawn (0.3.11)
1516
pry (0.10.3)
1617
coderay (~> 1.1.0)
1718
method_source (~> 0.8.1)
@@ -42,6 +43,7 @@ DEPENDENCIES
4243
concurrent-ruby (~> 1.0.0)
4344
flay!
4445
json
46+
posix-spawn
4547
pry
4648
rake
4749
rspec

lib/cc/engine/analyzers/command_line_runner.rb

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
require "open3"
1+
require "posix/spawn"
22
require "timeout"
33

44
module CC
@@ -13,13 +13,14 @@ def initialize(command, timeout = DEFAULT_TIMEOUT)
1313
end
1414

1515
def run(input)
16-
Timeout.timeout(timeout) do
17-
out, err, status = Open3.capture3(command, stdin_data: input)
18-
if status.success?
19-
yield out
20-
else
21-
raise ::CC::Engine::Analyzers::ParserError, "`#{command}` exited with code #{status.exitstatus}:\n#{err}"
22-
end
16+
child = POSIX::Spawn::Child.build(command, input: input, timeout: timeout)
17+
child.exec!
18+
19+
if child.success?
20+
yield child.out
21+
else
22+
exitstatus = child.status.exitstatus
23+
raise ::CC::Engine::Analyzers::ParserError, "`#{command}` exited with code #{exitstatus}:\n#{child.err}"
2324
end
2425
end
2526

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
require "spec_helper"
2+
require "cc/engine/duplication"
3+
4+
module CC::Engine::Analyzers
5+
RSpec.describe CommandLineRunner do
6+
describe "#run" do
7+
it "runs the command on the input and yields the output" do
8+
runner = CommandLineRunner.new("cat; echo hi")
9+
10+
output = runner.run("oh ") { |o| o }
11+
12+
expect(output).to eq "oh hi\n"
13+
end
14+
15+
16+
it "raises on errors" do
17+
runner = CommandLineRunner.new("echo error output >&2; false")
18+
19+
expect { runner.run("") }.to raise_error(
20+
ParserError, /code 1:\nerror output/
21+
)
22+
end
23+
24+
it "times out commands" do
25+
runner = CommandLineRunner.new("sleep 3", 0.01)
26+
27+
expect { runner.run("") }.to raise_error(POSIX::Spawn::TimeoutExceeded)
28+
end
29+
end
30+
end
31+
end

0 commit comments

Comments
 (0)