From 904f149e5b2f0ae9eff09dacb4f1ec41c4a76298 Mon Sep 17 00:00:00 2001 From: Kevin Lin Date: Tue, 4 Aug 2020 10:50:38 +0800 Subject: reverse_proxy: fix bidirectional streams with encodings (fix #3606) (#3620) * reverse_proxy: fix bi-h2stream breaking gzip encode handle(#3606). * reverse_proxy: check http version of both sides to avoid affecting non-h2 upstream. * Minor cleanup; apply review suggestions Co-authored-by: Matthew Holt --- modules/caddyhttp/reverseproxy/reverseproxy.go | 4 ++-- modules/caddyhttp/reverseproxy/streaming.go | 24 +++++++++++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) (limited to 'modules/caddyhttp') diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index 0a53db4..7fdf55a 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -613,8 +613,8 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, di Dia // some apps need the response headers before starting to stream content with http2, // so it's important to explicitly flush the headers to the client before streaming the data. - // (see https://github.com/caddyserver/caddy/issues/3556 for use case) - if req.ProtoMajor == 2 && res.ContentLength == -1 { + // (see https://github.com/caddyserver/caddy/issues/3556 for use case and nuances) + if h.isBidirectionalStream(req, res) { if wf, ok := rw.(http.Flusher); ok { wf.Flush() } diff --git a/modules/caddyhttp/reverseproxy/streaming.go b/modules/caddyhttp/reverseproxy/streaming.go index 105ff32..127c0f0 100644 --- a/modules/caddyhttp/reverseproxy/streaming.go +++ b/modules/caddyhttp/reverseproxy/streaming.go @@ -96,15 +96,33 @@ func (h Handler) flushInterval(req *http.Request, res *http.Response) time.Durat return -1 // negative means immediately } - // for h2 and h2c upstream streaming data to client (issue #3556) - if req.ProtoMajor == 2 && res.ContentLength == -1 { + // for h2 and h2c upstream streaming data to client (issues #3556 and #3606) + if h.isBidirectionalStream(req, res) { return -1 } - // TODO: more specific cases? e.g. res.ContentLength == -1? (this TODO is from the std lib) + // TODO: more specific cases? e.g. res.ContentLength == -1? (this TODO is from the std lib, but + // strangely similar to our isBidirectionalStream function that we implemented ourselves) return time.Duration(h.FlushInterval) } +// isBidirectionalStream returns whether we should work in bi-directional stream mode. +// +// See https://github.com/caddyserver/caddy/pull/3620 for discussion of nuances. +func (h Handler) isBidirectionalStream(req *http.Request, res *http.Response) bool { + // We have to check the encoding here; only flush headers with identity encoding. + // Non-identity encoding might combine with "encode" directive, and in that case, + // if body size larger than enc.MinLength, upper level encode handle might have + // Content-Encoding header to write. + // (see https://github.com/caddyserver/caddy/issues/3606 for use case) + ae := req.Header.Get("Accept-Encoding") + + return req.ProtoMajor == 2 && + res.ProtoMajor == 2 && + res.ContentLength == -1 && + (ae == "identity" || ae == "") +} + func (h Handler) copyResponse(dst io.Writer, src io.Reader, flushInterval time.Duration) error { if flushInterval != 0 { if wf, ok := dst.(writeFlusher); ok { -- cgit v1.2.3