[PATCH 0/5] proxy_pass resource cleanup fixes

[PATCH 0/5] proxy_pass resource cleanup fixes

[Eric Wong]

Lightly-tested, I'm going to let these run on YHBT.net for
a bit before cutting a new release.

And all this proxy_pass stuff could still use some cleanup and
refactoring, it's hairy!

Eric Wong (5):
      test_proxy_pass: test for auto chunking on 1.0 backends
      wbuf: drop persistence if writing to client fails
      proxy_http_response: cleanup: avoid redundant setting of "alive"
      proxy_http_response: do not persist upstream on slow clients
      proxy_pass: drop resources immediately on errors

 lib/yahns/proxy_http_response.rb | 37 +++++++++++++++++++++----------------
 lib/yahns/proxy_pass.rb          |  5 +++--
 lib/yahns/wbuf_common.rb         |  1 +
 test/test_proxy_pass.rb          | 15 +++++++++++++++
 4 files changed, 40 insertions(+), 18 deletions(-)

[PATCH 1/5] test_proxy_pass: test for auto chunking on 1.0 backends

[Eric Wong]

These are followups to the following two commits:

* commit d16326723d
  ("proxy_http_response: fix non-terminated fast responses, too")

* commit 8c9f33a539
  ("proxy_http_response: workaround non-terminated backends")
---
 test/test_proxy_pass.rb | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/test/test_proxy_pass.rb b/test/test_proxy_pass.rb
index 448e480..c938976 100644
--- a/test/test_proxy_pass.rb
+++ b/test/test_proxy_pass.rb
@@ -586,6 +586,21 @@ def check_eof_body(host, port)
       assert_match %r{\AHTTP/1\.1 200 OK\r\n}, res
       assert_match %r{\r\n\r\neof-body-slow\z}, res
       s.close
+
+      # we auto-chunk on 1.1 requests and 1.0 backends
+      %w(eof-body-slow eof-body-fast).each do |x|
+        s = TCPSocket.new(host, port)
+        s.write("GET /#{x} HTTP/1.1\r\nHost: example.com\r\n\r\n")
+        res = ''.dup
+        res << s.readpartial(512) until res =~ /0\r\n\r\n\z/
+        s.close
+        head, body = res.split("\r\n\r\n", 2)
+        head = head.split("\r\n")
+        assert_equal 'HTTP/1.1 200 OK', head[0]
+        assert head.include?('Connection: keep-alive')
+        assert head.include?('Transfer-Encoding: chunked')
+        assert_match %r{\Ad\r\n#{x}\r\n0\r\n\r\n\z}, body
+      end
     end
   end
 
-- 
EW

[PATCH 2/5] wbuf: drop persistence if writing to client fails

[Eric Wong]

We cannot maintain a persistent connection to a client if
writing to the client fails; so we can't proceed to let
the app hijack the response.

This may happen in the unlikely case where a response
header needs to be buffered with a Wbuf (and the app
uses response hijacking).
---
 lib/yahns/wbuf_common.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/yahns/wbuf_common.rb b/lib/yahns/wbuf_common.rb
index c51050b..ee18218 100644
--- a/lib/yahns/wbuf_common.rb
+++ b/lib/yahns/wbuf_common.rb
@@ -38,6 +38,7 @@ def wbuf_flush(client)
     end while @sf_count > 0
     wbuf_close(client)
   rescue
+    @wbuf_persist = false # ensure a hijack response is not called
     wbuf_close(client)
     raise
   end
-- 
EW

[PATCH 3/5] proxy_http_response: cleanup: avoid redundant setting of "alive"

[Eric Wong]

We already check for the truthiness of "alive" in the "if"
statement, so re-setting is pointless.
---
 lib/yahns/proxy_http_response.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index c8a2a42..e551323 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -101,7 +101,6 @@ def proxy_response_start(res, tip, kcar, req_res)
     # but the backend does not terminate properly
     if alive && ! term && (env['HTTP_VERSION'] == 'HTTP/1.1'.freeze)
       res << "Transfer-Encoding: chunked\r\n".freeze
-      alive = true
     end
     res << (alive ? "Connection: keep-alive\r\n\r\n".freeze
                   : "Connection: close\r\n\r\n".freeze)
-- 
EW

[PATCH 4/5] proxy_http_response: do not persist upstream on slow clients

[Eric Wong]

For slow clients, we want to be able to drop the connection
to the upstream as soon as we are done buffering and not waste
resources by leaving it in an :ignore state.  We also need to
remember the client for the fdmap to prevent shutdowns.

Ugh, this is really hard to test locally.
---
 lib/yahns/proxy_http_response.rb | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index e551323..3462e40 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -56,6 +56,7 @@ def wait_on_upstream(req_res, alive, wbuf)
     :wait_readable # self remains in :ignore, wait on upstream
   end
 
+  # start streaming the response once upstream is done sending headers to us.
   # returns :wait_readable if we need to read more from req_res
   # returns :ignore if we yield control to the client(self)
   # returns nil if completely done
@@ -177,9 +178,9 @@ def proxy_response_start(res, tip, kcar, req_res)
       end
     end
 
-    return proxy_busy_mod_done(alive) unless wbuf
-    req_res.resbuf = wbuf
-    proxy_busy_mod_blocked(wbuf, wbuf.busy)
+    # all done reading response from upstream, req_res will be discarded
+    # when we return nil:
+    wbuf ? proxy_busy_mod_blocked(wbuf, wbuf.busy) : proxy_busy_mod_done(alive)
   rescue => e
     proxy_err_response(502, req_res, e, wbuf)
   end
@@ -247,7 +248,7 @@ def proxy_response_finish(kcar, wbuf, req_res)
     end
 
     busy = wbuf.busy and return proxy_busy_mod_blocked(wbuf, busy)
-    proxy_busy_mod_done(wbuf.wbuf_persist) # returns nil
+    proxy_busy_mod_done(wbuf.wbuf_persist) # returns nil to close req_res
   end
 
   def proxy_wait_next(qflags)
@@ -288,23 +289,23 @@ def proxy_busy_mod_done(alive)
     when :close then close
     end
 
-    nil # close the req_res, too
+    nil # signal close for ReqRes#yahns_step
   end
 
   def proxy_busy_mod_blocked(wbuf, busy)
-    q = Thread.current[:yahns_queue]
     # we are completely done reading and buffering the upstream response,
     # but have not completely written the response to the client,
     # yield control to the client socket:
     @state = wbuf
-    case busy
-    when :wait_readable then q.queue_mod(self, Yahns::Queue::QEV_RD)
-    when :wait_writable then q.queue_mod(self, Yahns::Queue::QEV_WR)
-    else
-      abort "BUG: invalid wbuf.busy: #{busy.inspect}"
-    end
-    # no touching self after queue_mod
-    :ignore
+    proxy_wait_next(case busy
+      when :wait_readable then Yahns::Queue::QEV_RD
+      when :wait_writable then Yahns::Queue::QEV_WR
+      else
+        abort "BUG: invalid wbuf.busy: #{busy.inspect}"
+      end)
+    # no touching self after proxy_wait_next, we may be running
+    # HttpClient#yahns_step in a different thread at this point
+    nil # signal close for ReqRes#yahns_step
   end
 
   # n.b.: we can use String#size for optimized dispatch under YARV instead
-- 
EW

[PATCH 5/5] proxy_pass: drop resources immediately on errors

[Eric Wong]

We don't want to wait on GC to reap sockets on errors,
generational GC in Ruby is less aggressive about reaping
long-lived objects such as long-lived HTTP connections.
---
 lib/yahns/proxy_http_response.rb | 7 ++++++-
 lib/yahns/proxy_pass.rb          | 5 +++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/yahns/proxy_http_response.rb b/lib/yahns/proxy_http_response.rb
index 3462e40..c5e7be5 100644
--- a/lib/yahns/proxy_http_response.rb
+++ b/lib/yahns/proxy_http_response.rb
@@ -43,7 +43,12 @@ def proxy_err_response(code, req_res, exc, wbuf)
                      Rack::Utils::HTTP_STATUS_CODES[code]}\r\n\r\n") rescue nil
 
     shutdown rescue nil
-    req_res.shutdown rescue nil
+    @input = @input.close if @input
+
+    # this is safe ONLY because we are in an :ignore state after
+    # Fdmap#forget when we got hijacked:
+    close
+
     nil # signal close of req_res from yahns_step in yahns/proxy_pass.rb
   ensure
     wbuf.wbuf_abort if wbuf
diff --git a/lib/yahns/proxy_pass.rb b/lib/yahns/proxy_pass.rb
index 511db02..148957b 100644
--- a/lib/yahns/proxy_pass.rb
+++ b/lib/yahns/proxy_pass.rb
@@ -63,7 +63,8 @@ def yahns_step # yahns event loop entry point
 
         when Yahns::WbufCommon # streaming/buffering the response body
 
-          return c.proxy_response_finish(req, resbuf, self)
+          # we assign wbuf for rescue below:
+          return c.proxy_response_finish(req, wbuf = resbuf, self)
 
         end while true # case @resbuf
 
@@ -79,7 +80,7 @@ def yahns_step # yahns event loop entry point
       when Errno::ECONNREFUSED, Errno::ECONNRESET, Errno::EPIPE
         e.set_backtrace([])
       end
-      c.proxy_err_response(502, self, e, nil)
+      c.proxy_err_response(502, self, e, wbuf)
     end
 
     # returns :wait_readable if complete, :wait_writable if not
-- 
EW