[PATCH 0/5] exec_cgi: various cleanups and fixes

[PATCH 0/5] exec_cgi: various cleanups and fixes

[Eric Wong]

Yup, I still use this to run cgit...

      test/test_extras_exec_cgi.rb: improve test reliability
      extras/exec_cgi: remove kgio dependency
      extras/exec_cgi: update copyright year and use SPDX
      extras/exec_cgi: @body_tip is always set
      extras/exec_cgi: support Process.spawn options (e.g. RLIMIT_*)

 extras/exec_cgi.rb           | 60 +++++++++++++++++++++++++++-----------------
 test/test_extras_exec_cgi.rb | 25 +++++++++++++++++-
 2 files changed, 61 insertions(+), 24 deletions(-)

[PATCH 1/5] test/test_extras_exec_cgi.rb: improve test reliability

[Eric Wong]

Make the this test less multi-core/scheduler dependent.
---
 test/test_extras_exec_cgi.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/test_extras_exec_cgi.rb b/test/test_extras_exec_cgi.rb
index 3c71fd4..c6c5ad6 100644
--- a/test/test_extras_exec_cgi.rb
+++ b/test/test_extras_exec_cgi.rb
@@ -170,7 +170,7 @@ def _blocked_zombie(block_on, rtype)
         assert_match %r{\A\d+\n\z}, body
         exec_pid = body.to_i
         poke_until_dead exec_pid
-        assert_raises(EOFError) { c.read_nonblock(666) }
+        assert_raises(EOFError) { c.readpartial(666) }
       else
         raise "BUG in test, bad rtype"
       end
-- 
EW

[PATCH 2/5] extras/exec_cgi: remove kgio dependency

[Eric Wong]

We don't need non-blocking I/O at all in this module and it's
not coupled with the rest of yahns at all.
---
 extras/exec_cgi.rb | 46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/extras/exec_cgi.rb b/extras/exec_cgi.rb
index b546e1f..7fb5138 100644
--- a/extras/exec_cgi.rb
+++ b/extras/exec_cgi.rb
@@ -21,18 +21,29 @@
 #   run ExecCgi.new('/path/to/cgit.cgi') # cgit: https://git.zx2c4.com/cgit/
 #
 class ExecCgi
-  class MyIO < Kgio::Pipe
+  class MyIO
     attr_writer :my_pid
     attr_writer :body_tip
+    attr_reader :rd
+
+    def initialize(rd)
+      @rd = rd
+    end
 
     def each
       buf = @body_tip || ''.dup
       if buf.size > 0
         yield buf
       end
-      while tmp = kgio_read(8192, buf)
+
+      case tmp = @rd.read_nonblock(8192, buf, exception: false)
+      when :wait_readable
+        @rd.wait_readable
+      when nil
+        break
+      else # String
         yield tmp
-      end
+      end while true
       self
     ensure
       # do this sooner, since the response body may be buffered, we want
@@ -46,8 +57,8 @@ def close
       # Note: this object (and any client-specific objects) will never
       # be shared across different threads, so we do not need extra
       # mutual exclusion here.
-      return if closed?
-      super
+      return if @rd.closed?
+      @rd.close
       begin
         Process.waitpid(@my_pid)
       rescue Errno::ECHILD
@@ -90,20 +101,23 @@ def call(env)
     cgi_env = { "GATEWAY_INTERFACE" => "CGI/1.1" }
     PASS_VARS.each { |key| val = env[key] and cgi_env[key] = val }
     env.each { |key,val| cgi_env[key] = val if key =~ /\AHTTP_/ }
-    pipe = MyIO.pipe
-    errbody = pipe[0]
-    errbody.my_pid = Process.spawn(cgi_env.merge!(@env), *@args,
-                                   out: pipe[1], close_others: true)
-    pipe[1].close
-    pipe = pipe[0]
 
-    if head = pipe.kgio_read(8192)
+    rd, wr = IO.pipe
+    io = MyIO.new(rd)
+    errbody = io
+    errbody.my_pid = spawn(cgi_env.merge!(@env), *@args,
+                           out: wr, close_others: true)
+    wr.close
+
+    begin
+      head = rd.readpartial(8192)
       until head =~ /\r?\n\r?\n/
-        tmp = pipe.kgio_read(8192) or break
+        tmp = rd.readpartial(8192)
         head << tmp
+        tmp.clear
       end
       head, body = head.split(/\r?\n\r?\n/, 2)
-      pipe.body_tip = body
+      io.body_tip = body
 
       env["HTTP_VERSION"] ||= "HTTP/1.0" # stop Rack::Chunked for HTTP/0.9
 
@@ -117,8 +131,8 @@ def call(env)
       end
       status = headers.delete("Status") || 200
       errbody = nil
-      [ status, headers, pipe ]
-    else
+      [ status, headers, io ]
+    rescue EOFError
       [ 500, { "Content-Length" => "0", "Content-Type" => "text/plain" }, [] ]
     end
   ensure
-- 
EW

[PATCH 3/5] extras/exec_cgi: update copyright year and use SPDX

[Eric Wong]

Machine-parseability is useful for licenses.
---
 extras/exec_cgi.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/extras/exec_cgi.rb b/extras/exec_cgi.rb
index 7fb5138..f143ac7 100644
--- a/extras/exec_cgi.rb
+++ b/extras/exec_cgi.rb
@@ -1,6 +1,6 @@
 # -*- encoding: binary -*-
-# Copyright (C) 2013-2016 all contributors <yahns-public@yhbt.net>
-# License: GPLv2 or later (https://www.gnu.org/licenses/gpl-2.0.txt)
+# Copyright (C) 2013-2018 all contributors <yahns-public@yhbt.net>
+# License: GPL-2.0+ <https://www.gnu.org/licenses/gpl-2.0.txt>
 # frozen_string_literal: true
 #
 # if running under yahns, worker_processes is recommended to avoid conflicting
-- 
EW

[PATCH 4/5] extras/exec_cgi: @body_tip is always set

[Eric Wong]

No point in increasing the complexity for cases it's not.
---
 extras/exec_cgi.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/extras/exec_cgi.rb b/extras/exec_cgi.rb
index f143ac7..babda3c 100644
--- a/extras/exec_cgi.rb
+++ b/extras/exec_cgi.rb
@@ -31,10 +31,8 @@ def initialize(rd)
     end
 
     def each
-      buf = @body_tip || ''.dup
-      if buf.size > 0
-        yield buf
-      end
+      buf = @body_tip
+      yield buf unless buf.empty?
 
       case tmp = @rd.read_nonblock(8192, buf, exception: false)
       when :wait_readable
-- 
EW

[PATCH 5/5] extras/exec_cgi: support Process.spawn options (e.g. RLIMIT_*)

[Eric Wong]

These options can be useful for limiting CGI process runtime and
memory usage.
---
 extras/exec_cgi.rb           |  6 ++++--
 test/test_extras_exec_cgi.rb | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/extras/exec_cgi.rb b/extras/exec_cgi.rb
index babda3c..2746c09 100644
--- a/extras/exec_cgi.rb
+++ b/extras/exec_cgi.rb
@@ -18,7 +18,8 @@
 #   use Rack::Chunked
 #   # other Rack middlewares can go here...
 #
-#   run ExecCgi.new('/path/to/cgit.cgi') # cgit: https://git.zx2c4.com/cgit/
+#   # cgit: https://git.zx2c4.com/cgit/
+#   run ExecCgi.new('/path/to/cgit.cgi', opts)
 #
 class ExecCgi
   class MyIO
@@ -91,6 +92,7 @@ def initialize(*args)
     first[0] == ?/ or args[0] = ::File.expand_path(first)
     File.executable?(args[0]) or
       raise ArgumentError, "#{args[0]} is not executable"
+    @opts = Hash === args[-1] ? args.pop : {}
   end
 
   # Calls the app
@@ -104,7 +106,7 @@ def call(env)
     io = MyIO.new(rd)
     errbody = io
     errbody.my_pid = spawn(cgi_env.merge!(@env), *@args,
-                           out: wr, close_others: true)
+                           @opts.merge(out: wr, close_others: true))
     wr.close
 
     begin
diff --git a/test/test_extras_exec_cgi.rb b/test/test_extras_exec_cgi.rb
index c6c5ad6..48d62b7 100644
--- a/test/test_extras_exec_cgi.rb
+++ b/test/test_extras_exec_cgi.rb
@@ -179,4 +179,27 @@ def _blocked_zombie(block_on, rtype)
     c.close if c
     quit_wait(pid)
   end
+
+  def test_rlimit_options
+    err, cfg, host, port = @err, Yahns::Config.new, @srv.addr[3], @srv.addr[1]
+    tout = 1
+    opts = { rlimit_cpu: tout, rlimit_core: 0 }
+    cmd = [ '/bin/sh', '-c', 'while :; do :;done', opts ]
+    pid = mkserver(cfg) do
+      require './extras/exec_cgi'
+      cfg.instance_eval do
+        stack = Rack::ContentLength.new(Rack::Chunked.new(ExecCgi.new(*cmd)))
+        app(:rack, stack) { listen "#{host}:#{port}" }
+        stderr_path err.path
+        worker_processes 1
+      end
+    end
+    c = get_tcp_client(host, port)
+    c.write "GET / HTTP/1.0\r\n\r\n"
+    assert_same c, c.wait(tout + 0.5)
+    assert_match %r{ 500 Internal Server Error\b}, c.readpartial(4096)
+    c.close
+  ensure
+    quit_wait(pid)
+  end
 end
-- 
EW

Re: [PATCH 5/5] extras/exec_cgi: support Process.spawn options (e.g. RLIMIT_*)

[Eric Wong]

> diff --git a/test/test_extras_exec_cgi.rb b/test/test_extras_exec_cgi.rb
> index c6c5ad6..48d62b7 100644
> --- a/test/test_extras_exec_cgi.rb
> +++ b/test/test_extras_exec_cgi.rb

> +    assert_same c, c.wait(tout + 0.5)

That timeout needs to be increased for slower/overloaded
systems...