From a32d80a93101b884c44991247b8278002368d483 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Wed, 6 Nov 2013 17:59:25 +0000 Subject: 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. --- extras/exec_cgi.rb | 6 ++- lib/yahns/http_response.rb | 26 +++++------ test/server_helper.rb | 1 + test/test_extras_exec_cgi.rb | 104 ++++++++++++++++++++++++++++++++++++++++--- test/test_extras_exec_cgi.sh | 17 +++++++ 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 -- cgit v1.2.3-24-ge0c7