This repository has been archived by the owner. It is now read-only.
Permalink
Browse files

Don't use ClimateControl when it's not necessary

The ProcessRunner and PosixRunner are perfectly capable of taking an
environment hash. This means there's no reason to use ClimateControl to
modify the environment beforehand. In order to find custom binaries, we
can modify the PATH on the command line -- yes, for both Unix and
Windows.

Unix commandlines use the normal `PATH=/new/path:$PATH cmd` syntax.
Windows uses `SET PATH=C:\new\path;%PATH% & cmd` instead.

The difference between then and now is that I found out that you can
actually separate commands in Windows with `&` (see
http://support.microsoft.com/kb/279253).

As a result of this the `supplemental_environment` isn't modified when
the `path` is modified. We keep the path in memory as a
`Cocaine::OS.path_separator`-separated string.

In addition, this commit cleans up the OS detection slightly, moving it
to a new class. In the future, we can do things like telling instead of
asking.

The result of all this is that the `ProcessRunner` and `PosixRunner`
should be thread-safe, since we're not mucking around with the `ENV`
when we run stuff anymore. This will make the high-throughput users of
Paperclip happy, since they shouldn't see clobbering issues like they
did before. I tested this by running the code supplied by @maxim,
thoughtbot/paperclip#1709 (comment),
with the unmodified cocaine 0.5.4 gem and with this commit in place. It
failed in a few seconds before and ran for as long as I wanted after.

This also might fix a bug? Previously, on Java on Windows, commands
would be called prefixed with `env`, which doesn't exist on Windows. No
one complained, though, so you can guess how popular Java on Windows is
(or I'm completely screwing up).  Now we look for Windows first, then
use `env` with Java, and then Unix like normal.
  • Loading branch information...
Jon Yurek
Jon Yurek committed Dec 3, 2014
1 parent 7d917e1 commit e61a6782da3be57037a47415d0d0b4dee9cc2e39
@@ -180,6 +180,13 @@ try to use `PopenRunner`:
Cocaine::CommandLine.runner = Cocaine::CommandLine::PopenRunner.new
```

## Thread Safety

Cocaine should be thread safe. As discussed [here, in this climate_control
thread](https://github.com/thoughtbot/climate_control/pull/11), climate_control,
which modifies the environment under which commands are run for the
BackticksRunner and PopenRunner, is thread-safe but not reentrant. Please let us
know if you find this is ever not the case.

## REE

@@ -1,6 +1,7 @@
# coding: UTF-8

require 'rbconfig'
require 'cocaine/os_detector'
require 'cocaine/command_line'
require 'cocaine/command_line/runners'
require 'cocaine/exceptions'
@@ -10,9 +10,9 @@ def path
end

def path=(supplemental_path)
@supplemental_path = supplemental_path
@supplemental_environment ||= {}
@supplemental_environment['PATH'] = (Array(supplemental_path) + [ENV['PATH']]).join(File::PATH_SEPARATOR)
@supplemental_path = Array(supplemental_path).
flatten.
join(OS.path_separator)
end

def environment
@@ -35,10 +35,6 @@ def unfake!
@runner = nil
end

def java?
RUBY_PLATFORM =~ /java/
end

private

def best_runner
@@ -65,7 +61,7 @@ def initialize(binary, params = "", options = {})
end

def command(interpolations = {})
cmd = [@binary, interpolate(@params, interpolations)]
cmd = [path_prefix, @binary, interpolate(@params, interpolations)]
cmd << bit_bucket if @swallow_stderr
cmd.join(" ").strip
end
@@ -98,10 +94,6 @@ def run(interpolations = {})
output
end

def unix?
RbConfig::CONFIG['host_os'] !~ /mswin|mingw/
end

private

def colored(text, ansi_color = "\e[32m")
@@ -118,6 +110,28 @@ def log(text)
end
end

def path_prefix
if !self.class.path.nil? && !self.class.path.empty?
os_path_prefix
end
end

def os_path_prefix
if OS.unix?
unix_path_prefix
else
windows_path_prefix
end
end

def unix_path_prefix
"PATH=#{self.class.path}#{OS.path_separator}$PATH"
end

def windows_path_prefix
"SET PATH=#{self.class.path}#{OS.path_separator}%PATH% &"
end

def execute(command)
runner.call(command, environment, runner_options)
end
@@ -152,7 +166,7 @@ def shell_quote_all_values(values)

def shell_quote(string)
return "" if string.nil?
if unix?
if OS.unix?
if string.empty?
"''"
else
@@ -164,7 +178,7 @@ def shell_quote(string)
end

def bit_bucket
unix? ? "2>/dev/null" : "2>NUL"
OS.unix? ? "2>/dev/null" : "2>NUL"
end
end
end
@@ -22,13 +22,25 @@ def call(command, env = {}, options = {})
private

def env_command(command)
if Cocaine::CommandLine.java?
"env #{command}"
else
windows_command(command) || java_command(command) || default_command(command)
end

def windows_command(command)
if OS.windows?
command
end
end

def java_command(command)
if OS.java?
"env #{command}"
end
end

def default_command(command)
command
end

def with_modified_environment(env, &block)
ClimateControl.modify(env, &block)
end
@@ -11,7 +11,7 @@ def self.available?
end

def self.supported?
available? && !Cocaine::CommandLine.java?
available? && !OS.java?
end

def supported?
@@ -21,17 +21,15 @@ def supported?
def call(command, env = {}, options = {})
input, output = IO.pipe
options[:out] = output
with_modified_environment(env) do
pid = spawn(env, command, options)
output.close
result = ""
while partial_result = input.read(8192)
result << partial_result
end
waitpid(pid)
input.close
result
pid = spawn(env, command, options)
output.close
result = ""
while partial_result = input.read(8192)
result << partial_result
end
waitpid(pid)
input.close
result
end

private
@@ -44,10 +42,6 @@ def waitpid(pid)
Process.waitpid(pid)
end

def with_modified_environment(env, &block)
ClimateControl.modify(env, &block)
end

end
end
end
@@ -8,7 +8,7 @@ def self.available?
end

def self.supported?
available? && !Cocaine::CommandLine.java?
available? && !OS.java?
end

def supported?
@@ -18,14 +18,12 @@ def supported?
def call(command, env = {}, options = {})
input, output = IO.pipe
options[:out] = output
with_modified_environment(env) do
pid = spawn(env, command, options)
output.close
result = input.read
waitpid(pid)
input.close
result
end
pid = spawn(env, command, options)
output.close
result = input.read
waitpid(pid)
input.close
result
end

private
@@ -40,10 +38,6 @@ def waitpid(pid)
# In JRuby, waiting on a finished pid raises.
end

def with_modified_environment(env, &block)
ClimateControl.modify(env, &block)
end

end
end
end
@@ -0,0 +1,27 @@
# coding: UTF-8

module Cocaine
class OSDetector
def java?
arch =~ /java/
end

def unix?
RbConfig::CONFIG['host_os'] !~ /mswin|mingw/
end

def windows?
!unix?
end

def path_separator
File::PATH_SEPARATOR
end

def arch
RUBY_PLATFORM
end
end

OS = OSDetector.new
end
@@ -11,18 +11,17 @@
cmd.command.should == "convert a.jpg b.png"
end

it "specifies the $PATH where the command can be found" do
saved_path = ENV['PATH']
begin
ENV['PATH'] = "/the/environment/path:/other/bin"
Cocaine::CommandLine.path = "/path/to/command/dir"
cmd = Cocaine::CommandLine.new("echo", "$PATH")
cmd.command.should == "echo $PATH"
output = cmd.run
output.should match(%r{/path/to/command/dir:/the/environment/path:/other/bin})
ensure
ENV['PATH'] = saved_path
end
it "specifies the $PATH where the command can be found on unix" do
Cocaine::CommandLine.path = ["/path/to/command/dir", "/"]
cmd = Cocaine::CommandLine.new("ls")
cmd.command.should == "PATH=/path/to/command/dir:/:$PATH ls"
end

it "specifies the %PATH% where the command can be found on windows" do
on_windows!
Cocaine::CommandLine.path = ['C:\system32', 'D:\\']
cmd = Cocaine::CommandLine.new("dir")
cmd.command.should == 'SET PATH=C:\system32;D:\;%PATH% & dir'
end

it "specifies more than one path where the command can be found" do
@@ -152,20 +151,6 @@
end
end

it "detects that the system is unix" do
Cocaine::CommandLine.new("convert").should be_unix
end

it "detects that the system is windows" do
on_windows!
Cocaine::CommandLine.new("convert").should_not be_unix
end

it "detects that the system is windows (mingw)" do
on_mingw!
Cocaine::CommandLine.new("convert").should_not be_unix
end

it "colorizes the output to a tty" do
logger = FakeLogger.new(:tty => true)
Cocaine::CommandLine.new("echo", "'Logging!' :foo", :logger => logger).run(:foo => "bar")
@@ -0,0 +1,23 @@
require 'spec_helper'

describe Cocaine::OSDetector do
it "detects that the system is unix" do
on_unix!
Cocaine::OS.should be_unix
end

it "detects that the system is windows" do
on_windows!
Cocaine::OS.should be_windows
end

it "detects that the system is windows (mingw)" do
on_mingw!
Cocaine::OS.should be_windows
end

it "detects that the current Ruby is on Java" do
on_java!
Cocaine::OS.should be_java
end
end
@@ -72,7 +72,7 @@
end

after do
FileUtils.rm_f(Cocaine::CommandLine.path + 'ls')
FileUtils.rm_f("#{Cocaine::CommandLine.path}/ls")
end

[
@@ -1,14 +1,21 @@
module StubOS
def on_windows!
stub_os('mswin')
Cocaine::OS.stubs(:path_separator).returns(";")
end

def on_unix!
stub_os('darwin11.0.0')
Cocaine::OS.stubs(:path_separator).returns(":")
end

def on_mingw!
stub_os('mingw')
Cocaine::OS.stubs(:path_separator).returns(";")
end

def on_java!
Cocaine::OS.stubs(:arch).returns("universal-java1.7")
end

def stub_os(host_string)

0 comments on commit e61a678

Please sign in to comment.