From 9cd4a50c275bbda9ee23f0351f1eba2289af075f Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sat, 19 Oct 2013 02:47:58 +0000 Subject: fix and test Rack hijack support Rack hijacking may close the socket before it yields control back to our epoll event loop. So it's not safe to attempt to use the socket (or even get the .fileno) with :delete/EPOLL_CTL_DEL. So change :delete to :ignore, and only decrement the FD counter to allow yahns to do graceful shutdown. This means we'll potentially waste ~200 bytes of kernel memory due to epoll overhead until the FD is closed by the app hijacking. I'm not sure how Rack servers handle graceful shutdown when hijacking, but maybe they just retrap SIGQUIT... --- lib/yahns/fdmap.rb | 8 ----- lib/yahns/http_client.rb | 10 +++---- lib/yahns/http_response.rb | 6 ++-- lib/yahns/queue_epoll.rb | 5 ++-- lib/yahns/wbuf_common.rb | 2 +- test/test_queue.rb | 13 ++++---- test/test_rack_hijack.rb | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 93 insertions(+), 25 deletions(-) create mode 100644 test/test_rack_hijack.rb diff --git a/lib/yahns/fdmap.rb b/lib/yahns/fdmap.rb index 1e1677a..d83898b 100644 --- a/lib/yahns/fdmap.rb +++ b/lib/yahns/fdmap.rb @@ -54,14 +54,6 @@ class Yahns::Fdmap # :nodoc: @fdmap_mtx.synchronize { @count -= 1 } end - def delete(io) # use with rack.hijack (via yahns) - fd = io.fileno - @fdmap_mtx.synchronize do - @fdmap_ary[fd] = nil - @count -= 1 - end - end - # expire a bunch of idle clients and register the current one # We should not be calling this too frequently, it is expensive # This is called while @fdmap_mtx is held diff --git a/lib/yahns/http_client.rb b/lib/yahns/http_client.rb index e95bb47..ce55525 100644 --- a/lib/yahns/http_client.rb +++ b/lib/yahns/http_client.rb @@ -33,9 +33,9 @@ class Yahns::HttpClient < Kgio::Socket # :nodoc: case rv = @state.wbuf_flush(self) when :wait_writable, :wait_readable return rv # tell epoll/kqueue to wait on this more - when :delete # :delete on hijack - @state = :delete - return :delete + when :ignore # :ignore on hijack + @state = :ignore + return :ignore when Yahns::StreamFile @state = rv # continue looping when true, false # done @@ -159,9 +159,9 @@ class Yahns::HttpClient < Kgio::Socket # :nodoc: # run the rack app response = k.app.call(env.merge!(k.app_defaults)) - return :delete if env.include?(RACK_HIJACK_IO) + return :ignore if env.include?(RACK_HIJACK_IO) - # this returns :wait_readable, :wait_writable, :delete, or nil: + # this returns :wait_readable, :wait_writable, :ignore, or nil: http_response_write(*response) end diff --git a/lib/yahns/http_response.rb b/lib/yahns/http_response.rb index b6228e5..3ee4e21 100644 --- a/lib/yahns/http_response.rb +++ b/lib/yahns/http_response.rb @@ -52,8 +52,8 @@ module Yahns::HttpResponse # :nodoc: case rv # trysendfile return value when nil case alive - when :delete - @state = :delete + when :ignore + @state = :ignore when true, false http_response_done(alive) end @@ -140,7 +140,7 @@ module Yahns::HttpResponse # :nodoc: if hijack hijack.call(self) - return :delete # trigger EPOLL_CTL_DEL + return :ignore end if body.respond_to?(:to_path) diff --git a/lib/yahns/queue_epoll.rb b/lib/yahns/queue_epoll.rb index c9febc4..5cb455b 100644 --- a/lib/yahns/queue_epoll.rb +++ b/lib/yahns/queue_epoll.rb @@ -35,9 +35,8 @@ class Yahns::Queue < SleepyPenguin::Epoll::IO # :nodoc: epoll_ctl(Epoll::CTL_MOD, io, QEV_WR) when :wait_readwrite epoll_ctl(Epoll::CTL_MOD, io, QEV_RDWR) - when :delete # only used by rack.hijack - epoll_ctl(Epoll::CTL_DEL, io, 0) - @fdmap.delete(io) + when :ignore # only used by rack.hijack + @fdmap.decr when nil # this is be the ONLY place where we call IO#close on # things inside the queue diff --git a/lib/yahns/wbuf_common.rb b/lib/yahns/wbuf_common.rb index e621311..20f6e18 100644 --- a/lib/yahns/wbuf_common.rb +++ b/lib/yahns/wbuf_common.rb @@ -24,7 +24,7 @@ module Yahns::WbufCommon # :nodoc: @body.close if @body.respond_to?(:close) if @wbuf_persist.respond_to?(:call) # hijack @wbuf_persist.call(client) - :delete + :ignore else @wbuf_persist # true or false or Yahns::StreamFile end diff --git a/test/test_queue.rb b/test/test_queue.rb index 6d61aef..cdb9ade 100644 --- a/test/test_queue.rb +++ b/test/test_queue.rb @@ -23,8 +23,8 @@ class TestQueue < Testcase def r.yahns_step begin case read_nonblock(11) - when "delete" - return :delete + when "ignore" + return :ignore end rescue Errno::EAGAIN return :wait_readable @@ -38,13 +38,16 @@ class TestQueue < Testcase @q.spawn_worker_threads(@logger, 1, 1) Thread.pass until r.nread == 0 - w.write("delete") + assert_equal 1, @fdmap.size + w.write("ignore") Thread.pass until r.nread == 0 Thread.pass until @fdmap.size == 0 - # should not raise - @q.queue_add(r, Yahns::Queue::QEV_RD) + assert_raises(Errno::EEXIST) { + @q.queue_add(r, Yahns::Queue::QEV_RD) + } assert_equal 1, @fdmap.size + @q.epoll_ctl(SleepyPenguin::Epoll::CTL_MOD, r, Yahns::Queue::QEV_RD) w.close Thread.pass until @fdmap.size == 0 end diff --git a/test/test_rack_hijack.rb b/test/test_rack_hijack.rb new file mode 100644 index 0000000..ba2bd78 --- /dev/null +++ b/test/test_rack_hijack.rb @@ -0,0 +1,74 @@ +# Copyright (C) 2013, Eric Wong and all contributors +# License: GPLv3 or later (https://www.gnu.org/licenses/gpl-3.0.txt) +require_relative 'server_helper' + +class TestRackHijack < Testcase + parallelize_me! + include ServerHelper + alias setup server_helper_setup + alias teardown server_helper_teardown + + class DieIfUsed + def each + abort "body.each called after response hijack\n" + end + + def close + abort "body.close called after response hijack\n" + end + end + + HIJACK_APP = lambda { |env| + case env["PATH_INFO"] + when "/hijack_req" + io = env["rack.hijack"].call + if io.respond_to?(:read_nonblock) && + env["rack.hijack_io"].respond_to?(:read_nonblock) + + # exercise both, since we Rack::Lint may use different objects + env["rack.hijack_io"].write("HTTP/1.0 200 OK\r\n\r\n") + io.write("request.hijacked") + io.close + return [ 500, {}, DieIfUsed.new ] + end + [ 500, {}, [ "hijack BAD\n" ] ] + when "/hijack_res" + r = "response.hijacked" + [ 200, + { + "X-Test" => "zzz", + "Content-Length" => r.bytesize.to_s, + "rack.hijack" => proc { |x| x.write(r); x.close } + }, + DieIfUsed.new + ] + end + } + + def test_hijack + err = @err + cfg = Yahns::Config.new + host, port = @srv.addr[3], @srv.addr[1] + cfg.instance_eval do + GTL.synchronize { app(:rack, HIJACK_APP) { listen "#{host}:#{port}" } } + logger(Logger.new(err.path)) + end + srv = Yahns::Server.new(cfg) + pid = fork do + ENV["YAHNS_FD"] = @srv.fileno.to_s + srv.start.join + end + res = Net::HTTP.start(host, port) { |h| h.get("/hijack_req") } + assert_equal "request.hijacked", res.body + assert_equal 200, res.code.to_i + assert_equal "1.0", res.http_version + + res = Net::HTTP.start(host, port) { |h| h.get("/hijack_res") } + assert_equal "response.hijacked", res.body + assert_equal 200, res.code.to_i + assert_equal "zzz", res["X-Test"] + assert_equal "1.1", res.http_version + ensure + quit_wait(pid) + end +end -- cgit v1.2.3-24-ge0c7