Switch CGI implementation to use ChildProcess for the following reasons:

* We are already using it for the PHP process management
* The API of ChildProcess is simpler than using popen, etc manually.
* It offer cross platform support.
* It offers more efficient mechanisms like POSIX_SPAWN which can even
  be enabled via the CHILDPROCES_POSIX_SPAWN environment variable.

Author: eric1234

lib/rack/legacy/cgi.rb

@@ -1,5 +1,3 @@
-require 'shellwords'
-
 class Rack::Legacy::Cgi
 
   # Will setup a new instance of the CGI middleware executing
@@ -33,69 +31,52 @@ def valid?(path)
 
   # Will run the given path with the given environment
   def run env, path
-    env['DOCUMENT_ROOT'] = @public_dir
-    env['SERVER_SOFTWARE'] = 'Rack Legacy'
-    status = 200
-    headers = {}
-    body = ''
+    # Setup CGI process
+    cgi = ChildProcess.build path
+    cgi.duplex = true
+    cgi.cwd = File.dirname path
 
-    IO.popen('-', 'r+') do |io|
-      if io.nil?  # Child
-        # Pass on all uppercase environment variables. Only uppercase
-        # since Rack uses lower case ones internally.
-        env.each do |key, value|
-          ENV[key] = value if
-            value.respond_to?(:to_str) && key =~ /^[A-Z_]+$/
-        end
-        exec path
-      else        # Parent
-        # Send request to CGI sub-process
-        io.write(env['rack.input'].read) if env['rack.input']
-        io.close_write
-
-        # Parse headers coming back from sub-process
-        until io.eof? || (line = io.readline.chomp) == ''
-          if line =~ /\s*\:\s*/
-            key, value = line.split(/\s*\:\s*/, 2)
-            if headers.has_key? key
-              headers[key] += "\n" + value
-            else
-              headers[key] = value
-            end
-          end
-        end
+    # Arrange CGI processes IO
+    cgi_out, cgi.io.stdout = IO.pipe
+    cgi.io.stderr = $stderr
 
-        # Get response and wait for process to complete.
-        body = io.read
-        Process.wait
+    # Config CGI environment
+    cgi.environment['DOCUMENT_ROOT'] = @public_dir
+    cgi.environment['SERVER_SOFTWARE'] = 'Rack Legacy'
+    env.each do |key, value|
+      cgi.environment[key] = value if
+        value.respond_to?(:to_str) && key =~ /^[A-Z_]+$/
+    end
 
-        # If there was an error throw it up the execution stack so
-        # someone can rescue to provide info to the right person.
-        unless $?.exitstatus == 0
-          # Build full command for easier debugging. Output to
-          # stderr to prevent user from getting too much information.
-          cmd = env.inject([path]) do |assignments, (key, value)|
-            assignments.unshift "#{key}=#{value.to_s.shellescape}" if
-              value.respond_to?(:to_str) && key =~ /^[A-Z_]+$/
-            assignments
-          end * ' '
-          $stderr.puts <<ERROR
-CGI exited with status #{$?.exitstatus}. The full command run was:
+    # Start running CGI
+    cgi.start
 
-#{cmd}
+    # Delegate IO to CGI process
+    cgi.io.stdin.write env['rack.input'].read if env['rack.input']
+    cgi.io.stdout.close
 
-ERROR
-          raise Rack::Legacy::ExecutionError
+    # Extract headers from output
+    headers = {}
+    until cgi_out.eof? || (line = cgi_out.readline.chomp) == ''
+      if line =~ /\s*\:\s*/
+        key, value = line.split /\s*\:\s*/, 2
+        if headers.has_key? key
+          headers[key] += "\n" + value
+        else
+          headers[key] = value
         end
-
       end
     end
 
-    # Extract status from sub-process if it is doing something other
-    # than a 200 response.
-    status = headers.delete('Status').to_i if headers.has_key? 'Status'
+    # Extract status from sub-process, default to 200
+    status = (headers.delete('Status') || 200).to_i
+
+    # Throw error if process crashed.
+    # NOTE: Process could still be running and crash later. This just
+    #       ensure we response correctly if it immmediately crashes
+    raise Rack::Legacy::ExecutionError if cgi.crashed?
 
-    # Send it all back to rack
-    [status, headers, [body]]
+    # Send status, headers and remaining IO back to rack
+    [status, headers, cgi_out]
   end
 end

test/unit/cgi_test.rb

@@ -15,40 +15,40 @@ def test_valid?
 
   def test_call
     assert_equal \
-      [200, {"Content-Type"=>"text/html", "Content-Length"=>"7"}, ['Success']],
-      app.call({'PATH_INFO' => 'success.cgi', 'REQUEST_METHOD' => 'GET'})
+      [200, {"Content-Type"=>"text/html", "Content-Length"=>"7"}, 'Success'],
+      call({'PATH_INFO' => 'success.cgi', 'REQUEST_METHOD' => 'GET'})
     assert_equal \
       [200, {"Content-Type"=>"text/html"}, 'Endpoint'],
-      app.call({'PATH_INFO' => 'missing.cgi'})
-    assert_equal [200, {}, ['']],
-      app.call({'PATH_INFO' => 'empty.cgi', 'REQUEST_METHOD' => 'GET'})
-    assert_equal [404, {"Content-Type"=>"text/html"}, ['']],
-      app.call({'PATH_INFO' => '404.cgi', 'REQUEST_METHOD' => 'GET'})
-    assert_equal [200, {"Content-Type"=>"text/html", 'Set-Cookie' => "cookie1\ncookie2"}, ['']],
-      app.call({'PATH_INFO' => 'dup_headers.cgi', 'REQUEST_METHOD' => 'GET'})
+      call({'PATH_INFO' => 'missing.cgi'})
+    assert_equal [200, {}, ''],
+      call({'PATH_INFO' => 'empty.cgi', 'REQUEST_METHOD' => 'GET'})
+    assert_equal [404, {"Content-Type"=>"text/html"}, ''],
+      call({'PATH_INFO' => '404.cgi', 'REQUEST_METHOD' => 'GET'})
+    assert_equal [200, {"Content-Type"=>"text/html", 'Set-Cookie' => "cookie1\ncookie2"}, ''],
+      call({'PATH_INFO' => 'dup_headers.cgi', 'REQUEST_METHOD' => 'GET'})
 
     assert_raises Rack::Legacy::ExecutionError do
       $stderr.reopen open('/dev/null', 'w')
-      app.call({'PATH_INFO' => 'error.cgi', 'REQUEST_METHOD' => 'GET'})
+      call({'PATH_INFO' => 'error.cgi', 'REQUEST_METHOD' => 'GET'})
       $stderr.reopen STDERR
     end
 
     assert_raises Rack::Legacy::ExecutionError do
       $stderr.reopen open('/dev/null', 'w')
-      app.call({'PATH_INFO' => 'syntax_error.cgi', 'REQUEST_METHOD' => 'GET'})
+      call({'PATH_INFO' => 'syntax_error.cgi', 'REQUEST_METHOD' => 'GET'})
       $stderr.reopen STDERR
     end
 
     assert_equal \
-      [200, {"Content-Type"=>"text/html", "Content-Length"=>"5"}, ['query']],
-      app.call({
+      [200, {"Content-Type"=>"text/html", "Content-Length"=>"5"}, 'query'],
+      call({
         'PATH_INFO' => 'param.cgi',
         'QUERY_STRING' => 'q=query',
         'REQUEST_METHOD' => 'GET'
       })
     assert_equal \
-      [200, {"Content-Type"=>"text/html", "Content-Length"=>"4"}, ['post']],
-      app.call({
+      [200, {"Content-Type"=>"text/html", "Content-Length"=>"4"}, 'post'],
+      call({
         'PATH_INFO' => 'param.cgi',
         'REQUEST_METHOD' => 'POST',
         'CONTENT_LENGTH' => '6',
@@ -62,8 +62,8 @@ def test_call
   end
 
   def test_environment
-    status, headers, body = *app.call({'PATH_INFO' => 'env.cgi', 'REQUEST_METHOD' => 'GET'})
-    env = eval(body[0])
+    status, headers, body = *call({'PATH_INFO' => 'env.cgi', 'REQUEST_METHOD' => 'GET'})
+    env = eval body
     assert File.join(File.dirname(__FILE__), '../fixtures'), env['DOCUMENT_ROOT']
     assert 'Rack Legacy', env['SERVER_SOFTWARE']
   end
@@ -74,9 +74,15 @@ def fixture_file path
     File.expand_path path, File.join(File.dirname(__FILE__), '../fixtures')
   end
 
+  def call env
+    status, headers, body = *app.call(env)
+    body = body.read
+    [status, headers, body]
+  end
+
   def app
-    Rack::Legacy::Cgi.new \
-      proc {[200, {'Content-Type' => 'text/html'}, 'Endpoint']},
+    @app ||= Rack::Legacy::Cgi.new \
+      proc {[200, {'Content-Type' => 'text/html'}, StringIO.new('Endpoint')]},
       File.join(File.dirname(__FILE__), '../fixtures')
   end