* [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 related [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 related [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 related [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 related [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 related [flat|nested] 6+ messages in thread