Merge branch '1472-simpler-external-process-management' into rails-3-develop

7 files changed, 54 insertions, 37 deletions

diff --git a/Gemfile b/Gemfile
index 6db026194..3de506bcd 100644
--- a/Gemfile
+++ b/Gemfile
@@ -21,6 +21,7 @@ gem 'mahoro', '~> 0.4'
 gem 'memcache-client', '~> 1.8.5'
 gem 'net-http-local', '~> 0.1.2', :platforms => [:ruby_18, :ruby_19]
 gem 'net-purge', '~> 0.1.0'
+gem 'open4', '~> 1.3.4'
 gem 'rack', '~> 1.4.5'
 gem 'rake', '0.9.2.2'
 gem 'rails-i18n', '~> 0.7.3'
diff --git a/Gemfile.lock b/Gemfile.lock
index 3f900d047..b307f12ed 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -170,6 +170,7 @@ GEM
       net-ssh (>= 2.6.5)
     newrelic_rpm (3.9.7.266)
     nokogiri (1.5.9)
+    open4 (1.3.4)
     paper_trail (2.7.2)
       activerecord (~> 3.0)
       railties (~> 3.0)
@@ -331,6 +332,7 @@ DEPENDENCIES
   net-purge (~> 0.1.0)
   newrelic_rpm
   nokogiri (~> 1.5.9)
+  open4 (~> 1.3.4)
   pg (~> 0.17.1)
   pry (~> 0.9.6)
   quiet_assets (~> 1.0.2)
diff --git a/commonlib b/commonlib
-Subproject 4d0fd6d01bb34d3d592a2bb4adaa5927a50d892
+Subproject a26c614b2f34187d57a842bae70a278db75964c
diff --git a/lib/alaveteli_external_command.rb b/lib/alaveteli_external_command.rb
index 086a461c8..ddf968f90 100644
--- a/lib/alaveteli_external_command.rb
+++ b/lib/alaveteli_external_command.rb
@@ -5,61 +5,72 @@ module AlaveteliExternalCommand
         # Final argument can be a hash of options.
         # Valid options are:
         # :append_to - string to append the output of the process to
+        # :append_errors_to - string to append the errors produced by the process to
         # :stdin_string - stdin string to pass to the process
-        # :binary_output - boolean flag for treating the output as binary or text (only significant
-        #                  ruby 1.9 and above)
+        # :binary_output - boolean flag for treating the output as binary or text encoded with
+        #                   the default external encoding (only significant in ruby 1.9 and above)
+        # :binary_input - boolean flag for treating the input as binary or as text encoded with
+        #                   the default external encoding (only significant in ruby 1.9 and above)
         # :memory_limit - maximum amount of memory (in bytes) available to the process
+        # :timeout - maximum amount of time (in s) to allow the process to run for
+        # :env - hash of environment variables to set for the process
         def run(program_name, *args)
             # Run an external program, and return its output.
             # Standard error is suppressed unless the program
             # fails (i.e. returns a non-zero exit status).
+            # If the program fails, returns nil and writes any error to stderr.
+            # TODO: calling code should be able to specify error stream - may want to log it or
+            # otherwise act upon it.
             opts = {}
-            if !args.empty? && args[-1].is_a?(Hash)
-                opts = args.pop
-            end
-
-            if program_name =~ %r(^/)
-                program_path = program_name
-            else
-                found = false
-                AlaveteliConfiguration::utility_search_path.each do |d|
-                    program_path = File.join(d, program_name)
-                    if File.file? program_path and File.executable? program_path
-                        found = true
-                        break
-                    end
-                end
-                 raise "Could not find #{program_name} in any of #{AlaveteliConfiguration::utility_search_path.join(', ')}" if !found
+            if !args.empty? && args.last.is_a?(Hash)
+                opts = args.last
             end
 
+            program_path = find_program(program_name)
             xc = ExternalCommand.new(program_path, *args)
-            if opts.has_key? :append_to
-                xc.out = opts[:append_to]
-            end
-            if opts.has_key? :binary_output
-                xc.binary_mode = opts[:binary_output]
-            end
-            if opts.has_key? :memory_limit
-                xc.memory_limit = opts[:memory_limit]
+            begin
+                xc.run
+            rescue ExternalCommand::ChildUnterminated => e
+                $stderr.puts(e.message)
+                return nil
             end
-            xc.run(opts[:stdin_string] || "", opts[:env] || {})
 
             if !xc.exited
                 # Crash or timeout
-                $stderr.puts("#{program_name} #{args.join(' ')}:exited abnormally")
+                if xc.timed_out
+                    $stderr.puts(%Q[External Command: "#{program_name} #{args.join(' ')}" timed out at #{opts[:timeout]}s])
+                else
+                    $stderr.puts(%Q[External Command: "#{program_name} #{args.join(' ')}" exited abnormally])
+                end
+                $stderr.print(xc.err)
                 return nil
+
             elsif xc.status != 0
                 # Error
-                $stderr.puts("Error from #{program_name} #{args.join(' ')}:")
+                $stderr.puts(%Q[External Command: Error from command "#{program_name} #{args.join(' ')}":])
                 $stderr.print(xc.err)
                 return nil
             else
                 if opts.has_key? :append_to
                     opts[:append_to] << "\n\n"
                 else
+
                     return xc.out
                 end
             end
         end
+
+        def find_program(program_name)
+            if program_name =~ %r(^/)
+                return program_name
+            else
+                search_path = AlaveteliConfiguration::utility_search_path
+                search_path.each do |d|
+                    program_path = File.join(d, program_name)
+                    return program_name if File.file? program_path and File.executable? program_path
+                end
+                raise "Could not find #{program_name} in any of #{search_path.join(', ')}"
+            end
+        end
     end
 end
diff --git a/lib/mail_handler/mail_handler.rb b/lib/mail_handler/mail_handler.rb
index 47015f207..33d939e22 100644
--- a/lib/mail_handler/mail_handler.rb
+++ b/lib/mail_handler/mail_handler.rb
@@ -78,7 +78,9 @@ module MailHandler
             tempfile.binmode
             tempfile.print body
             tempfile.flush
-            default_params = { :append_to => text, :binary_output => false }
+            default_params = { :append_to => text,
+                               :binary_output => false,
+                               :timeout => 1200 }
             if content_type == 'application/vnd.ms-word'
                 AlaveteliExternalCommand.run("wvText", tempfile.path, tempfile.path + ".txt",
                                              { :memory_limit => 536870912,  :timeout => 120 } )
diff --git a/spec/script/handle-mail-replies_spec.rb b/spec/script/handle-mail-replies_spec.rb
index 90a8de27c..62d5c1dab 100644
--- a/spec/script/handle-mail-replies_spec.rb
+++ b/spec/script/handle-mail-replies_spec.rb
@@ -3,9 +3,9 @@ require "external_command"
 
 def mail_reply_test(email_filename)
     Dir.chdir Rails.root do
-        xc = ExternalCommand.new("script/handle-mail-replies", "--test")
-        xc.run(load_file_fixture(email_filename))
-
+        xc = ExternalCommand.new("script/handle-mail-replies", "--test",
+                                 :stdin_string => load_file_fixture(email_filename))
+        xc.run
         xc.err.should == ""
         return xc
     end
@@ -13,8 +13,9 @@ end
 
 describe "When filtering" do
     it "should not fail when not in test mode" do
-        xc = ExternalCommand.new("script/handle-mail-replies")
-        xc.run(load_file_fixture("track-response-exim-bounce.email"))
+        xc = ExternalCommand.new("script/handle-mail-replies",
+                                 { :stdin_string => load_file_fixture("track-response-exim-bounce.email") })
+        xc.run
         xc.err.should == ""
     end
 
diff --git a/spec/script/mailin_spec.rb b/spec/script/mailin_spec.rb
index 46ad39f7f..0ff094c2b 100644
--- a/spec/script/mailin_spec.rb
+++ b/spec/script/mailin_spec.rb
@@ -3,12 +3,12 @@ require "external_command"
 
 def mailin_test(email_filename)
     Dir.chdir Rails.root do
-        xc = ExternalCommand.new("script/mailin")
+
         mail = load_file_fixture(email_filename)
         ir = info_requests(:other_request)
         mail.gsub!('EMAIL_TO', ir.incoming_email)
         mail.gsub!('EMAIL_FROM', 'responder@localhost')
-        xc.run(mail)
+        xc = ExternalCommand.new("script/mailin", :stdin_string => mail).run
         xc.err.should == ""
         return xc
     end