[PATCH 0/3] maybe the last round of proxy_pass fixes before 1.13

[PATCH 0/3] maybe the last round of proxy_pass fixes before 1.13

[Eric Wong]

There's a lot of duplication added, but having to deal with
OpenSSL in currently-released versions of Ruby is painful
(as are slow clients trying to download gigantic responses :P)

Will let this run for a week or so before cutting 1.13

Eric Wong (3):
      test_proxy_pass_no_buffering: exclude rb/ru files, too
      wbuf_lite: use StringIO instead of TmpIO
      wbuf_lite: truncate StringIO when done

 lib/yahns/http_client.rb             | 26 ++++++++++++
 lib/yahns/openssl_client.rb          |  6 +--
 lib/yahns/sendfile_compat.rb         | 24 -----------
 lib/yahns/wbuf_common.rb             |  8 ----
 lib/yahns/wbuf_lite.rb               | 82 +++++++++++++++++++++++++++++++++---
 test/test_proxy_pass_no_buffering.rb |  4 +-
 test/test_tmpio.rb                   |  4 ++
 test/test_wbuf.rb                    |  4 ++
 8 files changed, 114 insertions(+), 44 deletions(-)

[PATCH 1/3] test_proxy_pass_no_buffering: exclude rb/ru files, too

[Eric Wong]

We may have temporary files lingering from concurrent
multi-threaded tests in our forked child since FD_CLOFORK
does not exist :P
---
 test/test_proxy_pass_no_buffering.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/test_proxy_pass_no_buffering.rb b/test/test_proxy_pass_no_buffering.rb
index 0afa4e1..2dc3b0b 100644
--- a/test/test_proxy_pass_no_buffering.rb
+++ b/test/test_proxy_pass_no_buffering.rb
@@ -106,7 +106,7 @@ def test_proxy_pass_no_buffering
           deleted2 = `lsof -p #{pid2}`.split("\n")
           deleted2 = deleted2.grep(/\bREG\b.*#{qtmpdir}.* \(deleted\)/)
           [ deleted1, deleted2 ].each do |ary|
-            ary.delete_if { |x| x =~ /\.(?:err|out) \(deleted\)/ }
+            ary.delete_if { |x| x =~ /\.(?:err|out|rb|ru) \(deleted\)/ }
           end
           assert_equal 1, deleted1.size, "pid1=#{deleted1.inspect}"
           assert_equal 0, deleted2.size, "pid2=#{deleted2.inspect}"
-- 
EW

[PATCH 2/3] wbuf_lite: use StringIO instead of TmpIO

[Eric Wong]

This allows us to work transparently with our OpenSSL
workaround[*] while allowing us to reuse our non-sendfile
compatibility code.  Unfortunately, this means we duplicate a
lot of code from the normal wbuf code for now; but that should
be fairly stable at this point.

[*] https://bugs.ruby-lang.org/issues/12085
---
 lib/yahns/http_client.rb             | 26 ++++++++++++
 lib/yahns/openssl_client.rb          |  6 +--
 lib/yahns/sendfile_compat.rb         | 24 ------------
 lib/yahns/wbuf_common.rb             |  8 ----
 lib/yahns/wbuf_lite.rb               | 76 ++++++++++++++++++++++++++++++++----
 test/test_proxy_pass_no_buffering.rb |  2 +-
 test/test_tmpio.rb                   |  4 ++
 test/test_wbuf.rb                    |  4 ++
 8 files changed, 107 insertions(+), 43 deletions(-)
 delete mode 100644 lib/yahns/sendfile_compat.rb

diff --git a/lib/yahns/http_client.rb b/lib/yahns/http_client.rb
index 1d64e08..1cdaa0f 100644
--- a/lib/yahns/http_client.rb
+++ b/lib/yahns/http_client.rb
@@ -2,6 +2,12 @@
 # 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
+begin
+  raise LoadError, 'SENDFILE_BROKEN env set' if ENV['SENDFILE_BROKEN']
+  require 'sendfile'
+rescue LoadError
+end
+
 class Yahns::HttpClient < Kgio::Socket # :nodoc:
   NULL_IO = StringIO.new(''.dup) # :nodoc:
 
@@ -304,4 +310,24 @@ def app_hijacked?(env, body)
     body.close if body.respond_to?(:close)
     true
   end
+
+  def trysendio(io, offset, count)
+    return 0 if count == 0
+    count = 0x4000 if count > 0x4000
+    buf = Thread.current[:yahns_sfbuf] ||= ''.dup
+    io.pos = offset
+    str = io.read(count, buf) or return # nil for EOF
+    n = 0
+    case rv = kgio_trywrite(str)
+    when String # partial write, keep trying
+      n += (str.size - rv.size)
+      str = rv
+    when :wait_writable, :wait_readable
+      return n > 0 ? n : rv
+    when nil
+      return n + str.size # yay!
+    end while true
+  end
+
+  alias trysendfile trysendio unless IO.instance_methods.include?(:trysendfile)
 end
diff --git a/lib/yahns/openssl_client.rb b/lib/yahns/openssl_client.rb
index f896acd..439bc75 100644
--- a/lib/yahns/openssl_client.rb
+++ b/lib/yahns/openssl_client.rb
@@ -3,8 +3,6 @@
 # License: GPL-3.0+ (https://www.gnu.org/licenses/gpl-3.0.txt)
 # frozen_string_literal: true
 
-require_relative 'sendfile_compat'
-
 # this is to be included into a Kgio::Socket-derived class
 # this requires Ruby 2.1 and later for "exception: false"
 module Yahns::OpenSSLClient # :nodoc:
@@ -72,7 +70,7 @@ def kgio_tryread(len, buf)
     @ssl.read_nonblock(len, buf, exception: false)
   end
 
-  def trysendfile(io, offset, count)
+  def trysendio(io, offset, count)
     return 0 if count == 0
 
     unless buf = @ssl_blocked
@@ -97,6 +95,8 @@ def shutdown # we never call this with a how=SHUT_* arg
     @ssl.sysclose
   end
 
+  alias trysendfile trysendio
+
   def close
     @ssl.close # flushes SSLSocket
     super # IO#close
diff --git a/lib/yahns/sendfile_compat.rb b/lib/yahns/sendfile_compat.rb
deleted file mode 100644
index 8bd4622..0000000
--- a/lib/yahns/sendfile_compat.rb
+++ /dev/null
@@ -1,24 +0,0 @@
-# -*- encoding: binary -*-
-# 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
-
-module Yahns::SendfileCompat # :nodoc:
-  def trysendfile(io, offset, count)
-    return 0 if count == 0
-    count = 0x4000 if count > 0x4000
-    buf = Thread.current[:yahns_sfbuf] ||= ''.dup
-    io.pos = offset
-    str = io.read(count, buf) or return # nil for EOF
-    n = 0
-    case rv = kgio_trywrite(str)
-    when String # partial write, keep trying
-      n += (str.size - rv.size)
-      str = rv
-    when :wait_writable, :wait_readable
-      return n > 0 ? n : rv
-    when nil
-      return n + str.size # yay!
-    end while true
-  end
-end
diff --git a/lib/yahns/wbuf_common.rb b/lib/yahns/wbuf_common.rb
index 2799baf..7cd19f7 100644
--- a/lib/yahns/wbuf_common.rb
+++ b/lib/yahns/wbuf_common.rb
@@ -2,14 +2,6 @@
 # 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
-begin
-  raise LoadError, "SENDFILE_BROKEN env set" if ENV["SENDFILE_BROKEN"]
-  require 'sendfile'
-rescue LoadError
-  require_relative 'sendfile_compat'
-  IO.__send__ :include, Yahns::SendfileCompat
-end
-
 module Yahns::WbufCommon # :nodoc:
   # returns true / false for persistent/non-persistent connections
   # returns :wait_*able when blocked
diff --git a/lib/yahns/wbuf_lite.rb b/lib/yahns/wbuf_lite.rb
index 25daf21..2da5349 100644
--- a/lib/yahns/wbuf_lite.rb
+++ b/lib/yahns/wbuf_lite.rb
@@ -2,30 +2,88 @@
 # 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
-require_relative 'wbuf'
+require 'stringio'
+require_relative 'wbuf_common'
 
 # This is only used for "proxy_buffering: false"
-class Yahns::WbufLite < Yahns::Wbuf # :nodoc:
+class Yahns::WbufLite # :nodoc:
+  include Yahns::WbufCommon
   attr_reader :busy
   attr_writer :req_res
 
   def initialize(req_res)
-    alive = req_res.alive
-    super(nil, alive ? :ignore : false)
+    @tmpio = nil
+    @sf_offset = @sf_count = 0
+    @wbuf_persist = :ignore
+    @busy = false
     @req_res = req_res
   end
 
-  def wbuf_write(client, buf)
-    super
+  def wbuf_write(c, buf)
+    buf = buf.join if Array === buf
+    # try to bypass the VFS layer and write directly to the socket
+    # if we're all caught up
+    case rv = c.kgio_trywrite(buf)
+    when String
+      buf = rv # retry in loop
+    when nil
+      return # yay! hopefully we don't have to buffer again
+    when :wait_writable, :wait_readable
+      @busy = rv
+    end until @busy
+
+    @tmpio ||= StringIO.new(''.dup) # relies on encoding: binary above
+    @sf_count += @tmpio.write(buf)
+
+    # we spent some time copying to the FS, try to write to
+    # the socket again in case some space opened up...
+    case rv = c.trysendio(@tmpio, @sf_offset, @sf_count)
+    when Integer
+      @sf_count -= rv
+      @sf_offset += rv
+    when :wait_writable, :wait_readable
+      @busy = rv
+      return rv
+    else
+      raise "BUG: #{rv.nil? ? "EOF" : rv.inspect} on tmpio " \
+            "sf_offset=#@sf_offset sf_count=#@sf_count"
+    end while @sf_count > 0
+
+    # we're all caught up, try to prevent dirty data from getting flushed
+    # to disk if we can help it.
+    wbuf_abort
+    @sf_offset = 0
+    @busy = false
+    nil
   rescue
     @req_res = @req_res.close if @req_res
     raise
   end
 
   def wbuf_flush(client)
-    super
+    case rv = client.trysendio(@tmpio, @sf_offset, @sf_count)
+    when Integer
+      return wbuf_close(client) if (@sf_count -= rv) == 0 # all sent!
+      @sf_offset += rv # keep going otherwise
+    when :wait_writable, :wait_readable
+      return rv
+    when nil
+      # response got truncated, drop the connection
+      # this may happens when using Rack::File or similar, we can't
+      # keep the connection alive because we already sent our Content-Length
+      # header the client would be confused.
+      @wbuf_persist = false
+      return wbuf_close(client)
+    else
+      raise "BUG: rv=#{rv.inspect} " \
+            "on tmpio=#{@tmpio.inspect} " \
+            "sf_offset=#@sf_offset sf_count=#@sf_count"
+    end while @sf_count > 0
+    wbuf_close(client)
   rescue
+    @wbuf_persist = false # ensure a hijack response is not called
     @req_res = @req_res.close if @req_res
+    wbuf_close(client)
     raise
   end
 
@@ -48,4 +106,8 @@ def wbuf_close(client)
     @req_res = @req_res.close if @req_res
     raise
   end
+
+  def wbuf_abort
+    @tmpio = @tmpio.close if @tmpio
+  end
 end
diff --git a/test/test_proxy_pass_no_buffering.rb b/test/test_proxy_pass_no_buffering.rb
index 2dc3b0b..356623f 100644
--- a/test/test_proxy_pass_no_buffering.rb
+++ b/test/test_proxy_pass_no_buffering.rb
@@ -108,7 +108,7 @@ def test_proxy_pass_no_buffering
           [ deleted1, deleted2 ].each do |ary|
             ary.delete_if { |x| x =~ /\.(?:err|out|rb|ru) \(deleted\)/ }
           end
-          assert_equal 1, deleted1.size, "pid1=#{deleted1.inspect}"
+          assert_equal 0, deleted1.size, "pid1=#{deleted1.inspect}"
           assert_equal 0, deleted2.size, "pid2=#{deleted2.inspect}"
           bufs.push(deleted1[0])
         end
diff --git a/test/test_tmpio.rb b/test/test_tmpio.rb
index 7d25d3f..3bcf3ca 100644
--- a/test/test_tmpio.rb
+++ b/test/test_tmpio.rb
@@ -5,6 +5,10 @@
 require_relative 'helper'
 
 class TestTmpIO < Testcase
+  def setup
+    skip 'sendfile missing' unless IO.instance_methods.include?(:sendfile)
+  end
+
   def test_writev
     a, b = UNIXSocket.pair
     a.extend Kgio::PipeMethods
diff --git a/test/test_wbuf.rb b/test/test_wbuf.rb
index 1382086..89825db 100644
--- a/test/test_wbuf.rb
+++ b/test/test_wbuf.rb
@@ -7,6 +7,10 @@
 class TestWbuf < Testcase
   ENV["N"].to_i > 1 and parallelize_me!
 
+  def setup
+    skip 'sendfile missing' unless IO.instance_methods.include?(:sendfile)
+  end
+
   class KgioUS < UNIXSocket
     include Kgio::SocketMethods
     def self.output_buffer_tmpdir
-- 
EW

[PATCH 3/3] wbuf_lite: truncate StringIO when done

[Eric Wong]

And explain why this is doable for StringIO and not TmpIO,
which is file-backed.
---
 lib/yahns/wbuf_lite.rb | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/yahns/wbuf_lite.rb b/lib/yahns/wbuf_lite.rb
index 2da5349..1902ce7 100644
--- a/lib/yahns/wbuf_lite.rb
+++ b/lib/yahns/wbuf_lite.rb
@@ -108,6 +108,12 @@ def wbuf_close(client)
   end
 
   def wbuf_abort
-    @tmpio = @tmpio.close if @tmpio
+    # we can safely truncate since this is a StringIO, we cannot do this
+    # with a real file because zero-copy with sendfile means truncating
+    # a while could clobber in-flight data
+    if @tmpio
+      @tmpio.truncate(0)
+      @tmpio = @tmpio.close
+    end
   end
 end
-- 
EW