[PATCH 0/4] remove chunked/Content-Length requirement from apps

[PATCH 0/4] remove chunked/Content-Length requirement from apps

[Eric Wong]

This better aligns with behavior of other real-world Rack
servers such as puma and Thin.  We will also now use kgio writev
functionality to speed up writing "Transfer-Encoding: chunked"
to avoid string copies or extra syscalls for other chunking
solutions.

See [PATCH 3/4] for a small informal benchmark.

4 changes
  response: drop clients after HTTP responses of unknown length
  response: reduce stack overhead for parameter passing
  response: support auto-chunking for HTTP/1.1
  Revert "document Rack::Chunked/ContentLength semi-requirements"

 Documentation/yahns-rackup.pod    | 10 -------
 examples/yahns_rack_basic.conf.rb |  6 -----
 lib/yahns/chunk_body.rb           | 27 +++++++++++++++++++
 lib/yahns/http_client.rb          | 24 ++++++++---------
 lib/yahns/http_response.rb        | 47 +++++++++++++++++++++++---------
 test/test_auto_chunk.rb           | 56 +++++++++++++++++++++++++++++++++++++++
 test/test_extras_exec_cgi.rb      |  4 +--
 7 files changed, 130 insertions(+), 44 deletions(-)
 create mode 100644 lib/yahns/chunk_body.rb
 create mode 100644 test/test_auto_chunk.rb

-- 
EW

[PATCH 1/4] response: drop clients after HTTP responses of unknown length

[Eric Wong]

Clients are not able to handle persistent connections unless
the client knows the length of the response.
---
 lib/yahns/http_response.rb   | 6 ++++++
 test/test_extras_exec_cgi.rb | 4 +---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/yahns/http_response.rb b/lib/yahns/http_response.rb
index 88ff9f9..f2d9c62 100644
--- a/lib/yahns/http_response.rb
+++ b/lib/yahns/http_response.rb
@@ -126,6 +126,7 @@ def http_response_write(status, headers, body, hdr_only)
     k = self.class
     alive = @hs.next? && k.persistent_connections
     flags = MSG_DONTWAIT
+    term = false
 
     if @hs.headers?
       code = status.to_i
@@ -147,14 +148,19 @@ def http_response_write(status, headers, body, hdr_only)
           # allow Rack apps to tell us they want to drop the client
           alive = false if value =~ /\bclose\b/i
         when %r{\AContent-Length\z}i
+          term = true
           flags |= MSG_MORE if value.to_i > 0 && !hdr_only
           kv_str(buf, key, value)
+        when %r{\ATransfer-Encoding\z}i
+          term = true if value =~ /\bchunked\b/i
+          kv_str(buf, key, value)
         when "rack.hijack"
           hijack = value
         else
           kv_str(buf, key, value)
         end
       end
+      alive &&= term
       buf << (alive ? "Connection: keep-alive\r\n\r\n".freeze
                     : "Connection: close\r\n\r\n".freeze)
       case rv = kgio_syssend(buf, flags)
diff --git a/test/test_extras_exec_cgi.rb b/test/test_extras_exec_cgi.rb
index f9b96d9..3c71fd4 100644
--- a/test/test_extras_exec_cgi.rb
+++ b/test/test_extras_exec_cgi.rb
@@ -169,10 +169,8 @@ def _blocked_zombie(block_on, rtype)
         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) }
+        assert_raises(EOFError) { c.read_nonblock(666) }
       else
         raise "BUG in test, bad rtype"
       end
-- 
EW

[PATCH 2/4] response: reduce stack overhead for parameter passing

[Eric Wong]

This makes it easier to add more parameters to
http_response_write and simplifies current callers.
---
 lib/yahns/http_client.rb   | 20 ++++++++++----------
 lib/yahns/http_response.rb |  7 ++++---
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/lib/yahns/http_client.rb b/lib/yahns/http_client.rb
index 873dd73..7a1bac1 100644
--- a/lib/yahns/http_client.rb
+++ b/lib/yahns/http_client.rb
@@ -191,9 +191,9 @@ def r100_done
     else # :lazy, false
       env = @hs.env
       hdr_only = env['REQUEST_METHOD'] == 'HEAD'.freeze
-      status, headers, body = k.app.call(env)
-      return :ignore if app_hijacked?(env, body)
-      http_response_write(status, headers, body, hdr_only)
+      res = k.app.call(env)
+      return :ignore if app_hijacked?(env, res)
+      http_response_write(res, hdr_only)
     end
   end
 
@@ -224,15 +224,15 @@ def app_call(input)
 
     hdr_only = env['REQUEST_METHOD'] == 'HEAD'.freeze
     # run the rack app
-    status, headers, body = k.app.call(env)
-    return :ignore if app_hijacked?(env, body)
-    if status.to_i == 100
+    res = k.app.call(env)
+    return :ignore if app_hijacked?(env, res)
+    if res[0].to_i == 100
       rv = http_100_response(env) and return rv
-      status, headers, body = k.app.call(env)
+      res = k.app.call(env)
     end
 
     # this returns :wait_readable, :wait_writable, :ignore, or nil:
-    http_response_write(status, headers, body, hdr_only)
+    http_response_write(res, hdr_only)
   end
 
   # called automatically by kgio_write
@@ -308,9 +308,9 @@ def handle_error(e)
     return # always drop the connection on uncaught errors
   end
 
-  def app_hijacked?(env, body)
+  def app_hijacked?(env, res)
     return false unless env.include?('rack.hijack_io'.freeze)
-    body.close if body.respond_to?(:close)
+    res[2].close if res && res[2].respond_to?(:close)
     true
   end
 
diff --git a/lib/yahns/http_response.rb b/lib/yahns/http_response.rb
index f2d9c62..b157ee4 100644
--- a/lib/yahns/http_response.rb
+++ b/lib/yahns/http_response.rb
@@ -8,8 +8,8 @@
 # Writes a Rack response to your client using the HTTP/1.1 specification.
 # You use it by simply doing:
 #
-#   status, headers, body = rack_app.call(env)
-#   http_response_write(status, headers, body, env['REQUEST_METHOD']=='HEAD')
+#   res = rack_app.call(env)
+#   http_response_write(res, env['REQUEST_METHOD']=='HEAD')
 #
 # Most header correctness (including Content-Length and Content-Type)
 # is the job of Rack, with the exception of the "Date" header.
@@ -120,7 +120,8 @@ def kv_str(buf, key, value)
 
   # writes the rack_response to socket as an HTTP response
   # returns :wait_readable, :wait_writable, :forget, or nil
-  def http_response_write(status, headers, body, hdr_only)
+  def http_response_write(res, hdr_only)
+    status, headers, body = res
     offset = 0
     count = hijack = nil
     k = self.class
-- 
EW

[PATCH 3/4] response: support auto-chunking for HTTP/1.1

[Eric Wong]

We might as well do it since puma and thin both do(*),
and we can still do writev for now to get some speedups
by avoiding Rack::Chunked overhead.

timing runs of "curl --no-buffer http://127.0.0.1:9292/ >/dev/null"
results in a best case drop from ~260ms to ~205ms on one VM
by disabling Rack::Chunked in the below config.ru

$ ruby -I lib bin/yahns-rackup -E none config.ru

==> config.ru <==
class Body
  STR = ' ' * 1024 * 16
  def each
    10000.times { yield STR }
  end
end

use Rack::Chunked if ENV['RACK_CHUNKED']
run(lambda do |env|
  [ 200, [ %w(Content-Type text/plain) ], Body.new ]
end)

(*) they can do Content-Length, but I don't think it's
    worth the effort at the server level.
---
 lib/yahns/chunk_body.rb    | 27 ++++++++++++++++++++++
 lib/yahns/http_client.rb   |  8 +++----
 lib/yahns/http_response.rb | 38 +++++++++++++++++++++----------
 test/test_auto_chunk.rb    | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 16 deletions(-)
 create mode 100644 lib/yahns/chunk_body.rb
 create mode 100644 test/test_auto_chunk.rb

diff --git a/lib/yahns/chunk_body.rb b/lib/yahns/chunk_body.rb
new file mode 100644
index 0000000..6e56a18
--- /dev/null
+++ b/lib/yahns/chunk_body.rb
@@ -0,0 +1,27 @@
+# -*- encoding: binary -*-
+# Copyright (C) 2016 all contributors <yahns-public@yhbt.net>
+# License: GPL-3.0+ <https://www.gnu.org/licenses/gpl-3.0.txt>
+# frozen_string_literal: true
+class Yahns::ChunkBody
+  def initialize(body, vec)
+    @body = body
+    @vec = vec
+  end
+
+  def each
+    vec = @vec
+    vec[2] = "\r\n".freeze
+    @body.each do |chunk|
+      vec[0] = "#{chunk.bytesize.to_s(16)}\r\n"
+      vec[1] = chunk
+      # vec[2] never changes: "\r\n" above
+      yield vec
+    end
+    vec.clear
+    yield "0\r\n\r\n".freeze
+  end
+
+  def close
+    @body.close if @body.respond_to?(:close)
+  end
+end
diff --git a/lib/yahns/http_client.rb b/lib/yahns/http_client.rb
index 7a1bac1..d8154a4 100644
--- a/lib/yahns/http_client.rb
+++ b/lib/yahns/http_client.rb
@@ -190,10 +190,10 @@ def r100_done
       true
     else # :lazy, false
       env = @hs.env
-      hdr_only = env['REQUEST_METHOD'] == 'HEAD'.freeze
+      opt = http_response_prep(env)
       res = k.app.call(env)
       return :ignore if app_hijacked?(env, res)
-      http_response_write(res, hdr_only)
+      http_response_write(res, opt)
     end
   end
 
@@ -222,7 +222,7 @@ def app_call(input)
       env['SERVER_PORT'] = '443'.freeze
     end
 
-    hdr_only = env['REQUEST_METHOD'] == 'HEAD'.freeze
+    opt = http_response_prep(env)
     # run the rack app
     res = k.app.call(env)
     return :ignore if app_hijacked?(env, res)
@@ -232,7 +232,7 @@ def app_call(input)
     end
 
     # this returns :wait_readable, :wait_writable, :ignore, or nil:
-    http_response_write(res, hdr_only)
+    http_response_write(res, opt)
   end
 
   # called automatically by kgio_write
diff --git a/lib/yahns/http_response.rb b/lib/yahns/http_response.rb
index b157ee4..32d1a45 100644
--- a/lib/yahns/http_response.rb
+++ b/lib/yahns/http_response.rb
@@ -4,12 +4,14 @@
 # frozen_string_literal: true
 require_relative 'stream_file'
 require_relative 'wbuf_str'
+require_relative 'chunk_body'
 
 # Writes a Rack response to your client using the HTTP/1.1 specification.
 # You use it by simply doing:
 #
+#   opt = http_response_prep(env)
 #   res = rack_app.call(env)
-#   http_response_write(res, env['REQUEST_METHOD']=='HEAD')
+#   http_response_write(res, opt)
 #
 # Most header correctness (including Content-Length and Content-Type)
 # is the job of Rack, with the exception of the "Date" header.
@@ -120,14 +122,14 @@ def kv_str(buf, key, value)
 
   # writes the rack_response to socket as an HTTP response
   # returns :wait_readable, :wait_writable, :forget, or nil
-  def http_response_write(res, hdr_only)
+  def http_response_write(res, opt)
     status, headers, body = res
     offset = 0
     count = hijack = nil
-    k = self.class
-    alive = @hs.next? && k.persistent_connections
+    alive = @hs.next? && self.class.persistent_connections
     flags = MSG_DONTWAIT
     term = false
+    hdr_only, chunk_ok = opt
 
     if @hs.headers?
       code = status.to_i
@@ -161,6 +163,11 @@ def http_response_write(res, hdr_only)
           kv_str(buf, key, value)
         end
       end
+      if !term && chunk_ok
+        term = true
+        body = Yahns::ChunkBody.new(body, opt)
+        buf << "Transfer-Encoding: chunked\r\n".freeze
+      end
       alive &&= term
       buf << (alive ? "Connection: keep-alive\r\n\r\n".freeze
                     : "Connection: close\r\n\r\n".freeze)
@@ -173,7 +180,7 @@ def http_response_write(res, hdr_only)
         flags = MSG_DONTWAIT
         buf = rv # unlikely, hope the skb grows
       when :wait_writable, :wait_readable # unlikely
-        if k.output_buffering
+        if self.class.output_buffering
           alive = hijack ? hijack : alive
           rv = response_header_blocked(buf, body, alive, offset, count)
           body = nil # ensure we do not close body in ensure
@@ -193,19 +200,19 @@ def http_response_write(res, hdr_only)
     end
 
     wbuf = rv = nil
-    body.each do |chunk|
+    body.each do |x|
       if wbuf
-        rv = wbuf.wbuf_write(self, chunk)
+        rv = wbuf.wbuf_write(self, x)
       else
-        case rv = kgio_trywrite(chunk)
+        case rv = String === x ? kgio_trywrite(x) : kgio_trywritev(x)
         when nil # all done, likely and good!
           break
-        when String
-          chunk = rv # hope the skb grows when we loop into the trywrite
+        when String, Array
+          x = rv # hope the skb grows when we loop into the trywrite
         when :wait_writable, :wait_readable
-          if k.output_buffering
+          if self.class.output_buffering
             wbuf = Yahns::Wbuf.new(body, alive)
-            rv = wbuf.wbuf_write(self, chunk)
+            rv = wbuf.wbuf_write(self, x)
             break
           else
             response_wait_write(rv) or return :close
@@ -278,4 +285,11 @@ def http_100_response(env)
       return rv
     end while true
   end
+
+  # must be called before app dispatch, since the app can
+  # do all sorts of nasty things to env
+  def http_response_prep(env)
+    [ env['REQUEST_METHOD'] == 'HEAD'.freeze, # hdr_only
+      env['HTTP_VERSION'] == 'HTTP/1.1'.freeze ] # chunk_ok
+  end
 end
diff --git a/test/test_auto_chunk.rb b/test/test_auto_chunk.rb
new file mode 100644
index 0000000..a97fe26
--- /dev/null
+++ b/test/test_auto_chunk.rb
@@ -0,0 +1,56 @@
+# Copyright (C) 2013-2016 all contributors <yahns-public@yhbt.net>
+# License: GPL-3.0+ <https://www.gnu.org/licenses/gpl-3.0.txt>
+# frozen_string_literal: true
+require_relative 'server_helper'
+
+class TestAutoChunk < Testcase
+  ENV["N"].to_i > 1 and parallelize_me!
+  include ServerHelper
+  alias setup server_helper_setup
+  alias teardown server_helper_teardown
+
+  def test_auto_head
+    err = @err
+    cfg = Yahns::Config.new
+    host, port = @srv.addr[3], @srv.addr[1]
+    cfg.instance_eval do
+      GTL.synchronize do
+        app = Rack::Builder.new do
+          use Rack::ContentType, "text/plain"
+          run(lambda do |env|
+            [ 200, {}, %w(a b c) ]
+          end)
+        end
+        app(:rack, app) { listen "#{host}:#{port}" }
+      end
+      logger(Logger.new(err.path))
+    end
+    pid = mkserver(cfg)
+    s = TCPSocket.new(host, port)
+    s.write("GET / HTTP/1.0\r\n\r\n")
+    assert s.wait(30), "IO wait failed"
+    buf = s.read
+    assert_match %r{\r\n\r\nabc\z}, buf
+    s.close
+
+    s = TCPSocket.new(host, port)
+    s.write("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n")
+    buf = ''.dup
+    Timeout.timeout(30) do
+      until buf =~ /\r\n\r\n1\r\na\r\n1\r\nb\r\n1\r\nc\r\n0\r\n\r\n\z/
+        buf << s.readpartial(16384)
+      end
+    end
+    assert_match(%r{^Transfer-Encoding: chunked\r\n}, buf)
+    s.close
+
+    Net::HTTP.start(host, port) do |http|
+      req = Net::HTTP::Get.new("/")
+      res = http.request(req)
+      assert_equal 200, res.code.to_i
+      assert_equal 'abc', res.body
+    end
+  ensure
+    quit_wait(pid)
+  end
+end
-- 
EW

[PATCH 4/4] Revert "document Rack::Chunked/ContentLength semi-requirements"

[Eric Wong]

Actually, I guess I misread, rack (starting at) 1.0 stopped
requiring Content-Length/Chunked headers but I never noticed.
Oh well.

This reverts commit 4968041a7e1ff90b920704f50fccb9e7968d0d99.
---
 Documentation/yahns-rackup.pod    | 10 ----------
 examples/yahns_rack_basic.conf.rb |  6 ------
 2 files changed, 16 deletions(-)

diff --git a/Documentation/yahns-rackup.pod b/Documentation/yahns-rackup.pod
index 6172661..efdfb6d 100644
--- a/Documentation/yahns-rackup.pod
+++ b/Documentation/yahns-rackup.pod
@@ -159,16 +159,6 @@ The RACK_ENV variable is set by the aforementioned -E switch.
 If RACK_ENV is already set, it will be used unless -E is used.
 See rackup documentation for more details.
 
-=head1 CAVEATS
-
-yahns is strict about buggy, non-compliant Rack applications.
-Some existing servers work fine without "Content-Length" or
-"Transfer-Encoding: chunked" response headers enforced by Rack::Lint.
-Forgetting these headers with yahns causes clients to stall as they
-assume more data is coming.  Loading the Rack::ContentLength and/or
-Rack::Chunked middlewares will set the necessary response headers
-and fix your app.
-
 =head1 CONTACT
 
 All feedback welcome via plain-text mail to L<mailto:yahns-public@yhbt.net>
diff --git a/examples/yahns_rack_basic.conf.rb b/examples/yahns_rack_basic.conf.rb
index 610a482..f3f8e6a 100644
--- a/examples/yahns_rack_basic.conf.rb
+++ b/examples/yahns_rack_basic.conf.rb
@@ -30,12 +30,6 @@
   worker_threads 50
 end
 
-# note: Rack requires responses set "Content-Length" or use
-# "Transfer-Encoding: chunked".  Some Rack servers tolerate
-# the lack of these, yahns does not.  Thus you should load
-# Rack::Chunked and/or Rack::ContentLength middleware in your
-# config.ru to ensure clients know when your application
-# responses terminate.
 app(:rack, "config.ru", preload: false) do
   listen 80
 
-- 
EW