http_response: reorder wbuf_maybe on successful early flush

We can use the wbuf_close return value instead to ensure we close
tmpio properly and follow the same code path as a normal
(:wait_writable-triggering) buffered response would.

Add a few tests to ensure we properly close the response body
for exec_cgi, where I noticed zombies and started me down this
rabbit hole looking for places where the response body was not
closed properly.

5 files changed, 133 insertions, 21 deletions

diff --git a/extras/exec_cgi.rb b/extras/exec_cgi.rb
index 083047e..170e773 100644
--- a/extras/exec_cgi.rb
+++ b/extras/exec_cgi.rb
@@ -67,7 +67,8 @@ class ExecCgi
     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
-    pipe[0].my_pid = Process.spawn(cgi_env, *@args,
+    errbody = pipe[0]
+    errbody.my_pid = Process.spawn(cgi_env, *@args,
                                    out: pipe[1], close_others: true)
     pipe[1].close
     pipe = pipe[0]
@@ -100,9 +101,12 @@ class ExecCgi
           pipe.chunked = true
         end
       end
+      errbody = nil
       [ status, headers, pipe ]
     else
       [ 500, { "Content-Length" => "0", "Content-Type" => "text/plain" }, [] ]
     end
+  ensure
+    errbody.close if errbody
   end
 end
diff --git a/lib/yahns/http_response.rb b/lib/yahns/http_response.rb
index a520216..8a2426b 100644
--- a/lib/yahns/http_response.rb
+++ b/lib/yahns/http_response.rb
@@ -50,24 +50,20 @@ module Yahns::HttpResponse # :nodoc:
     wbuf = Yahns::Wbuf.new(body, alive, self.class.output_buffer_tmpdir)
     rv = wbuf.wbuf_write(self, header)
     body.each { |chunk| rv = wbuf.wbuf_write(self, chunk) } if body
-    wbuf_maybe(wbuf, rv, alive)
+    wbuf_maybe(wbuf, rv)
   end
 
-  def wbuf_maybe(wbuf, rv, alive)
+  def wbuf_maybe(wbuf, rv)
     case rv # wbuf_write return value
     when nil # all done
-      begin
-        case alive
-        when :ignore # hijacked
-          @state = alive
-        when Yahns::StreamFile
-          @state = alive
-          :wait_writable
-        when true, false
-          http_response_done(alive)
-        end
-      ensure
-        wbuf.wbuf_close(self)
+      case rv = wbuf.wbuf_close(self)
+      when :ignore # hijacked
+        @state = rv
+      when Yahns::StreamFile
+        @state = rv
+        :wait_writable
+      when true, false
+        http_response_done(rv)
       end
     else
       @state = wbuf
@@ -188,7 +184,7 @@ module Yahns::HttpResponse # :nodoc:
     # (or :wait_readable for SSL) and hit Yahns::HttpClient#step_write
     if wbuf
       body = nil # ensure we do not close the body in ensure
-      wbuf_maybe(wbuf, rv, alive)
+      wbuf_maybe(wbuf, rv)
     else
       http_response_done(alive)
     end
diff --git a/test/server_helper.rb b/test/server_helper.rb
index 2f9266a..d4b8f98 100644
--- a/test/server_helper.rb
+++ b/test/server_helper.rb
@@ -16,6 +16,7 @@ module ServerHelper
   end
 
   def poke_until_dead(pid)
+    assert_operator pid, :>, 0
     Timeout.timeout(10) do
       begin
         Process.kill(0, pid)
diff --git a/test/test_extras_exec_cgi.rb b/test/test_extras_exec_cgi.rb
index 403925b..4e1f5b3 100644
--- a/test/test_extras_exec_cgi.rb
+++ b/test/test_extras_exec_cgi.rb
@@ -7,17 +7,15 @@ class TestExtrasExecCGI < Testcase
   include ServerHelper
   alias setup server_helper_setup
   alias teardown server_helper_teardown
+  RUNME = "#{Dir.pwd}/test/test_extras_exec_cgi.sh"
 
   def test_exec_cgi
     err, cfg, host, port = @err, Yahns::Config.new, @srv.addr[3], @srv.addr[1]
-    runme = "#{Dir.pwd}/test/test_extras_exec_cgi.sh"
-    assert File.executable?(runme), "run test in project root"
+    assert File.executable?(RUNME), "run test in project root"
     pid = mkserver(cfg) do
       require './extras/exec_cgi'
       cfg.instance_eval do
-        app(:rack, ExecCgi.new(runme)) do
-          listen "#{host}:#{port}"
-        end
+        app(:rack, ExecCgi.new(RUNME)) { listen "#{host}:#{port}" }
         stderr_path err.path
       end
     end
@@ -75,7 +73,103 @@ class TestExtrasExecCGI < Testcase
       assert_nil body
       c.close
     end
+
+    Timeout.timeout(30) do # pid of executable
+      c = get_tcp_client(host, port)
+      c.write "GET /pid HTTP/1.0\r\n\r\n"
+      head, body = c.read.split(/\r\n\r\n/, 2)
+      assert_match %r{200 OK}, head
+      assert_match %r{\A\d+\n\z}, body
+      exec_pid = body.to_i
+      c.close
+      poke_until_dead exec_pid
+    end
+  ensure
+    quit_wait(pid)
+  end
+
+  def test_cgi_died
+    err, cfg, host, port = @err, Yahns::Config.new, @srv.addr[3], @srv.addr[1]
+    pid = mkserver(cfg) do
+      require './extras/exec_cgi'
+      cfg.instance_eval do
+        app(:rack, ExecCgi.new(RUNME)) { listen "#{host}:#{port}" }
+        stderr_path err.path
+      end
+    end
+    exec_pid_tmp = tmpfile(%w(exec_cgi .pid))
+    c = get_tcp_client(host, port)
+    Timeout.timeout(20) do
+      c.write "GET /die HTTP/1.0\r\nX-PID-DEST: #{exec_pid_tmp.path}\r\n\r\n"
+      head, body = c.read.split(/\r\n\r\n/, 2)
+      assert_match %r{500 Internal Server Error}, head
+      assert_match "", body
+      exec_pid = exec_pid_tmp.read
+      assert_match %r{\A(\d+)\n\z}, exec_pid
+      poke_until_dead exec_pid.to_i
+    end
+  ensure
+    exec_pid_tmp.close! if exec_pid_tmp
+    quit_wait(pid)
+  end
+
+  [ 9, 10, 11 ].each do |rtype|
+    [ 1, 2, 3 ].each do |block_on|
+      define_method("test_block_on_block_on_#{block_on}_rtype_#{rtype}") do
+        _blocked_zombie([block_on], rtype)
+      end
+    end
+  end
+
+  def _blocked_zombie(block_on, rtype)
+    err, cfg, host, port = @err, Yahns::Config.new, @srv.addr[3], @srv.addr[1]
+    pid = mkserver(cfg) do
+      $_tw_blocked = 0
+      $_tw_block_on = block_on
+      Yahns::HttpClient.__send__(:include, TrywriteBlocked)
+      require './extras/exec_cgi'
+      cfg.instance_eval do
+        app(:rack, ExecCgi.new(RUNME)) { listen "#{host}:#{port}" }
+        stderr_path err.path
+      end
+    end
+
+    c = get_tcp_client(host, port)
+    Timeout.timeout(20) do
+      case rtype
+      when 9 # non-persistent (HTTP/0.9)
+        c.write "GET /pid\r\n\r\n"
+        body = c.read
+        assert_match %r{\A\d+\n\z}, body
+        exec_pid = body.to_i
+        poke_until_dead exec_pid
+      when 10 # non-persistent (HTTP/1.0)
+        c.write "GET /pid HTTP/1.0\r\n\r\n"
+        head, body = c.read.split(/\r\n\r\n/, 2)
+        assert_match %r{200 OK}, head
+        assert_match %r{\A\d+\n\z}, body
+        exec_pid = body.to_i
+        poke_until_dead exec_pid
+      when 11 # pid of executable, persistent
+        c.write "GET /pid HTTP/1.0\r\nConnection: keep-alive\r\n\r\n"
+        buf = ""
+        begin
+          buf << c.readpartial(666)
+        end until buf =~ /\r\n\r\n\d+\n/
+        head, body = buf.split(/\r\n\r\n/, 2)
+        assert_match %r{200 OK}, head
+        assert_match %r{\A\d+\n\z}, body
+        exec_pid = body.to_i
+        assert_raises(Errno::EAGAIN, IO::WaitReadable) { c.read_nonblock(666) }
+        poke_until_dead exec_pid
+        # still alive?
+        assert_raises(Errno::EAGAIN, IO::WaitReadable) { c.read_nonblock(666) }
+      else
+        raise "BUG in test, bad rtype"
+      end
+    end
   ensure
+    c.close if c
     quit_wait(pid)
   end
 end
diff --git a/test/test_extras_exec_cgi.sh b/test/test_extras_exec_cgi.sh
index e580773..b54a83f 100755
--- a/test/test_extras_exec_cgi.sh
+++ b/test/test_extras_exec_cgi.sh
@@ -20,6 +20,23 @@ case $PATH_INFO in
         stdhead
         env
         ;;
+/pid)
+        stdhead
+        echo $$
+        ;;
+/die)
+        if test -n "$HTTP_X_PID_DEST"
+        then
+                # obviously this is only for testing on a local machine:
+                echo $$ > "$HTTP_X_PID_DEST"
+                exit 1
+        else
+                echo Content-Type: text/plain
+                echo Status: 400 Bad Request
+                echo Content-Length: 0
+                echo
+        fi
+        ;;
 /known-length)
         echo Content-Type: text/plain
         echo Status: 200 OK