Permalink
Browse files

Abstract out what actually launches the command

Running a command with a modified environment creates a race condition
where the ENV can get clobbered. Process.spawn and POSIX::Spawn.spawn
get around this by allowing an env hash to be passed in, but they are
not available on 1.8. So, if they're available, use them, and if they're
not, fall back to the backticks.
  • Loading branch information...
1 parent 2cfec9c commit 94dbfa336751c11432c6cf0254357fad66802eca @jyurek jyurek committed Jun 29, 2012
View
@@ -1,4 +1,5 @@
require 'rbconfig'
+require 'cocaine/command_line/runners'
require 'cocaine/command_line'
require 'cocaine/exceptions'
@@ -1,11 +1,18 @@
module Cocaine
class CommandLine
- # Check for posix-spawn gem. If it is available it will prevent the invoked processes
- # from getting a copy of the ruby heap which can lead to significant performance gains.
+ class << self
+ attr_accessor :spawner
+ end
+
+ # Check for posix-spawn gem. If it is available it will prevent the
+ # invoked processes from getting a copy of the ruby heap which can
+ # lead to significant performance gains.
+
begin
- require 'posix/spawn'
+ require 'posix/spawn'
+ @@posix_spawn_available = true
rescue LoadError => e
- # posix-spawn gem not available
+ @@posix_spawn_available = false
end
class << self
@@ -20,13 +27,17 @@ def path=(supplemental_path)
@supplemental_environment['PATH'] = [ENV['PATH'], *supplemental_path].join(File::PATH_SEPARATOR)
end
+ def posix_spawn_available?
+ @@posix_spawn_available
+ end
+
def environment
@supplemental_environment ||= {}
end
end
@environment = {}
- attr_reader :exit_status
+ attr_reader :exit_status, :runner
def initialize(binary, params = "", options = {})
@binary = binary.dup
@@ -36,7 +47,7 @@ def initialize(binary, params = "", options = {})
@swallow_stderr = @options.delete(:swallow_stderr)
@expected_outcodes = @options.delete(:expected_outcodes)
@expected_outcodes ||= [0]
- extend(POSIX::Spawn) if defined?(POSIX::Spawn)
+ @runner = best_runner
end
def command
@@ -50,10 +61,8 @@ def command
def run
output = ''
begin
- with_modified_environment do
- @logger.info("\e[32mCommand\e[0m :: #{command}") if @logger
- output = send(:'`', command)
- end
+ @logger.info("\e[32mCommand\e[0m :: #{command}") if @logger
+ output = execute(command)
rescue Errno::ENOENT
raise Cocaine::CommandNotFoundError
ensure
@@ -74,14 +83,14 @@ def unix?
private
- def with_modified_environment
- begin
- saved_env = ENV.to_hash
- ENV.update(self.class.environment)
- yield
- ensure
- ENV.update(saved_env)
- end
+ def execute(command)
+ runner.call(command, self.class.environment)
+ end
+
+ def best_runner
+ return PosixRunner.new if self.class.posix_spawn_available?
+ return ProcessRunner.new if Process.respond_to?(:spawn)
+ BackticksRunner.new
end
def interpolate(pattern, vars)
@@ -0,0 +1,3 @@
+require 'cocaine/command_line/runners/backticks_runner'
+require 'cocaine/command_line/runners/process_runner'
+require 'cocaine/command_line/runners/posix_runner'
@@ -0,0 +1,25 @@
+module Cocaine
+ class CommandLine
+ class BackticksRunner
+
+ def call(command, env = {})
+ with_modified_environment(env) do
+ `#{command}`
+ end
+ end
+
+ private
+
+ def with_modified_environment(env)
+ begin
+ saved_env = ENV.to_hash
+ ENV.update(env)
+ yield
+ ensure
+ ENV.update(saved_env)
+ end
+ end
+
+ end
+ end
+end
@@ -0,0 +1,25 @@
+module Cocaine
+ class CommandLine
+ class PosixRunner
+
+ def call(command, env = {})
+ input, output = IO.pipe
+ pid = spawn(env, command, :out => output)
+ waitpid(pid)
+ output.close
+ input.read
+ end
+
+ private
+
+ def spawn(*args)
+ POSIX::Spawn.spawn(*args)
+ end
+
+ def waitpid(pid)
+ Process.waitpid(pid)
+ end
+
+ end
+ end
+end
@@ -0,0 +1,26 @@
+module Cocaine
+ class CommandLine
+ class ProcessRunner
+
+ def call(command, env = {})
+ input, output = IO.pipe
+ pid = spawn(env, command, :out => output)
+ waitpid(pid)
+ output.close
+ input.read
+ end
+
+ private
+
+ def spawn(*args)
+ Process.spawn(*args)
+ end
+
+ def waitpid(pid)
+ Process.waitpid(pid)
+ end
+
+ end
+ end
+end
+
@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe Cocaine::CommandLine::BackticksRunner do
+ it 'runs the command given' do
+ subject.call("echo hello").should == "hello\n"
+ end
+
+ it 'modifies the environment and runs the command given' do
+ subject.call("echo $yes", {"yes" => "no"}).should == "no\n"
+ end
+
+ it 'sets the exitstatus when a command completes' do
+ subject.call("ruby -e 'exit 0'")
+ $?.exitstatus.should == 0
+ subject.call("ruby -e 'exit 5'")
+ $?.exitstatus.should == 5
+ end
+end
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Cocaine::CommandLine::PosixRunner do
+ it 'runs the command given' do
+ subject.call("echo hello").should == "hello\n"
+ end
+
+ it 'modifies the environment and runs the command given' do
+ subject.call("echo $yes", {"yes" => "no"}).should == "no\n"
+ end
+
+ it 'sets the exitstatus when a command completes' do
+ subject.call("ruby -e 'exit 0'")
+ $?.exitstatus.should == 0
+ subject.call("ruby -e 'exit 5'")
+ $?.exitstatus.should == 5
+ end
+end
+
@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe Cocaine::CommandLine::ProcessRunner do
+ it 'runs the command given' do
+ subject.call("echo hello").should == "hello\n"
+ end
+
+ it 'modifies the environment and runs the command given' do
+ subject.call("echo $yes", {"yes" => "no"}).should == "no\n"
+ end
+
+ it 'sets the exitstatus when a command completes' do
+ subject.call("ruby -e 'exit 0'")
+ $?.exitstatus.should == 0
+ subject.call("ruby -e 'exit 5'")
+ $?.exitstatus.should == 5
+ end
+end
@@ -120,15 +120,15 @@
it "runs the command it's given and return the output" do
cmd = Cocaine::CommandLine.new("convert", "a.jpg b.png", :swallow_stderr => false)
- cmd.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
+ cmd.stubs(:execute).with("convert a.jpg b.png").returns(:correct_value)
with_exitstatus_returning(0) do
cmd.run.should == :correct_value
end
end
it "raises a CommandLineError if the result code from the command isn't expected" do
cmd = Cocaine::CommandLine.new("convert", "a.jpg b.png", :swallow_stderr => false)
- cmd.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
+ cmd.stubs(:execute).with("convert a.jpg b.png").returns(:correct_value)
with_exitstatus_returning(1) do
lambda do
cmd.run
@@ -141,7 +141,7 @@
"a.jpg b.png",
:expected_outcodes => [0, 1],
:swallow_stderr => false)
- cmd.stubs(:"`").with("convert a.jpg b.png").returns(:correct_value)
+ cmd.stubs(:execute).with("convert a.jpg b.png").returns(:correct_value)
with_exitstatus_returning(1) do
lambda do
cmd.run
@@ -151,7 +151,7 @@
it "should keep result code in #exitstatus" do
cmd = Cocaine::CommandLine.new("convert")
- cmd.stubs(:"`").with("convert").returns(:correct_value)
+ cmd.stubs(:execute).with("convert").returns(:correct_value)
with_exitstatus_returning(1) do
cmd.run rescue nil
end
@@ -191,31 +191,29 @@
cmd = Cocaine::CommandLine.new("echo", "'Logging!'", :logger => nil)
lambda { cmd.run }.should_not raise_error
end
-
+
describe "command execution" do
- it "should use the ` method to invoke the command line" do
+ it "uses the BackticksRunner by default" do
+ Process.stubs(:respond_to?).with(:spawn).returns(false)
+ Cocaine::CommandLine.stubs(:posix_spawn_available?).returns(false)
+
cmd = Cocaine::CommandLine.new("echo", "hello")
- cmd.stubs(:`).with(anything).returns(nil)
- cmd.run
- cmd.should have_received(:`).with("echo hello")
+ cmd.runner.class.should == Cocaine::CommandLine::BackticksRunner
end
-
- it "should use POSIX::Spawn to create processes if it is available" do
+
+ it "uses the ProcessRunner on 1.9 and it's available" do
+ Process.stubs(:respond_to?).with(:spawn).returns(true)
+ Cocaine::CommandLine.stubs(:posix_spawn_available?).returns(false)
+
cmd = Cocaine::CommandLine.new("echo", "hello")
- cmd.method(:`).owner.should == POSIX::Spawn
- cmd.run.chomp.should == "hello"
+ cmd.runner.class.should == Cocaine::CommandLine::ProcessRunner
end
-
- it "should use the default Kernel spawning to create processes if POSIX::Spawn is not available" do
- spawn = POSIX::Spawn
- POSIX.send(:remove_const, :Spawn)
- begin
- cmd = Cocaine::CommandLine.new("echo", "hello")
- cmd.method(:`).owner.should == Kernel
- cmd.run.chomp.should == "hello"
- ensure
- POSIX.const_set(:Spawn, spawn)
- end
+
+ it "uses the PosixRunner if the posix-spawn gem is available" do
+ Cocaine::CommandLine.stubs(:posix_spawn_available?).returns(true)
+
+ cmd = Cocaine::CommandLine.new("echo", "hello")
+ cmd.runner.class.should == Cocaine::CommandLine::PosixRunner
end
end
end

2 comments on commit 94dbfa3

@mike-burns
Member

I wish I knew what the race condition was. I suppose a reliable test for it is impossible?

@jyurek
Member
jyurek commented on 94dbfa3 Aug 29, 2012

It was. I mean, it's a race condition... when was the last time you could get a reliable test for one of those? Though I'm not unhappy with how this turned out regardless.

Please sign in to comment.