yahns Ruby server user/dev discussion
 help / color / Atom feed
* [PATCH 0/5] proxy_pass resource cleanup fixes
@ 2016-04-27  0:26 Eric Wong
  2016-04-27  0:27 ` [PATCH 1/5] test_proxy_pass: test for auto chunking on 1.0 backends Eric Wong
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Eric Wong @ 2016-04-27  0:26 UTC (permalink / raw)
  To: yahns-public

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(-)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/5] test_proxy_pass: test for auto chunking on 1.0 backends
  2016-04-27  0:26 [PATCH 0/5] proxy_pass resource cleanup fixes Eric Wong
@ 2016-04-27  0:27 ` Eric Wong
  2016-04-27  0:27 ` [PATCH 2/5] wbuf: drop persistence if writing to client fails Eric Wong
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-04-27  0:27 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/5] wbuf: drop persistence if writing to client fails
  2016-04-27  0:26 [PATCH 0/5] proxy_pass resource cleanup fixes Eric Wong
  2016-04-27  0:27 ` [PATCH 1/5] test_proxy_pass: test for auto chunking on 1.0 backends Eric Wong
@ 2016-04-27  0:27 ` Eric Wong
  2016-04-27  0:27 ` [PATCH 3/5] proxy_http_response: cleanup: avoid redundant setting of "alive" Eric Wong
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-04-27  0:27 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/5] proxy_http_response: cleanup: avoid redundant setting of "alive"
  2016-04-27  0:26 [PATCH 0/5] proxy_pass resource cleanup fixes Eric Wong
  2016-04-27  0:27 ` [PATCH 1/5] test_proxy_pass: test for auto chunking on 1.0 backends Eric Wong
  2016-04-27  0:27 ` [PATCH 2/5] wbuf: drop persistence if writing to client fails Eric Wong
@ 2016-04-27  0:27 ` Eric Wong
  2016-04-27  0:27 ` [PATCH 4/5] proxy_http_response: do not persist upstream on slow clients Eric Wong
  2016-04-27  0:27 ` [PATCH 5/5] proxy_pass: drop resources immediately on errors Eric Wong
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-04-27  0:27 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 4/5] proxy_http_response: do not persist upstream on slow clients
  2016-04-27  0:26 [PATCH 0/5] proxy_pass resource cleanup fixes Eric Wong
                   ` (2 preceding siblings ...)
  2016-04-27  0:27 ` [PATCH 3/5] proxy_http_response: cleanup: avoid redundant setting of "alive" Eric Wong
@ 2016-04-27  0:27 ` Eric Wong
  2016-04-27  0:27 ` [PATCH 5/5] proxy_pass: drop resources immediately on errors Eric Wong
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-04-27  0:27 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 5/5] proxy_pass: drop resources immediately on errors
  2016-04-27  0:26 [PATCH 0/5] proxy_pass resource cleanup fixes Eric Wong
                   ` (3 preceding siblings ...)
  2016-04-27  0:27 ` [PATCH 4/5] proxy_http_response: do not persist upstream on slow clients Eric Wong
@ 2016-04-27  0:27 ` Eric Wong
  4 siblings, 0 replies; 6+ messages in thread
From: Eric Wong @ 2016-04-27  0:27 UTC (permalink / raw)
  To: yahns-public

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-27  0:26 [PATCH 0/5] proxy_pass resource cleanup fixes Eric Wong
2016-04-27  0:27 ` [PATCH 1/5] test_proxy_pass: test for auto chunking on 1.0 backends Eric Wong
2016-04-27  0:27 ` [PATCH 2/5] wbuf: drop persistence if writing to client fails Eric Wong
2016-04-27  0:27 ` [PATCH 3/5] proxy_http_response: cleanup: avoid redundant setting of "alive" Eric Wong
2016-04-27  0:27 ` [PATCH 4/5] proxy_http_response: do not persist upstream on slow clients Eric Wong
2016-04-27  0:27 ` [PATCH 5/5] proxy_pass: drop resources immediately on errors Eric Wong

yahns Ruby server user/dev discussion

Archives are clonable:
	git clone --mirror https://yhbt.net/yahns-public
	git clone --mirror http://ou63pmih66umazou.onion/yahns-public

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.yahns
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.yahns

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox