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...

7 files changed, 93 insertions, 25 deletions

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 <normalperson@yhbt.net> 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