From 881b826fb59a25102fd14ae3b420639479f2d6bf Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 27 May 2020 11:42:19 -0600 Subject: reverseproxy: Pool copy buffers (minor optimization) --- modules/caddyhttp/reverseproxy/reverseproxy.go | 40 +++++--------------------- modules/caddyhttp/reverseproxy/streaming.go | 28 +++++++++--------- 2 files changed, 20 insertions(+), 48 deletions(-) (limited to 'modules') diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index 507995f..06802a0 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -587,20 +587,15 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, di Dia rw.WriteHeader(res.StatusCode) err = h.copyResponse(rw, res.Body, h.flushInterval(req, res)) + res.Body.Close() // close now, instead of defer, to populate res.Trailer if err != nil { - defer res.Body.Close() - // Since we're streaming the response, if we run into an error all we can do - // is abort the request. Issue golang/go#23643: ReverseProxy should use ErrAbortHandler - // on read error while copying body. - // TODO: Look into whether we want to panic at all in our case... - if !shouldPanicOnCopyError(req) { - // p.logf("suppressing panic for copyResponse error in test; copy error: %v", err) - return err - } - - panic(http.ErrAbortHandler) + // we're streaming the response and we've already written headers, so + // there's nothing an error handler can do to recover at this point; + // the standard lib's proxy panics at this point, but we'll just log + // the error and abort the stream here + h.logger.Error("aborting with incomplete response", zap.Error(err)) + return nil } - res.Body.Close() // close now, instead of defer, to populate res.Trailer if len(res.Trailer) > 0 { // Force chunking if we saw a response trailer. @@ -679,27 +674,6 @@ func (h Handler) directRequest(req *http.Request, di DialInfo) { req.URL.Host = reqHost } -// shouldPanicOnCopyError reports whether the reverse proxy should -// panic with http.ErrAbortHandler. This is the right thing to do by -// default, but Go 1.10 and earlier did not, so existing unit tests -// weren't expecting panics. Only panic in our own tests, or when -// running under the HTTP server. -// TODO: I don't know if we want this at all... -func shouldPanicOnCopyError(req *http.Request) bool { - // if inOurTests { - // // Our tests know to handle this panic. - // return true - // } - if req.Context().Value(http.ServerContextKey) != nil { - // We seem to be running under an HTTP server, so - // it'll recover the panic. - return true - } - // Otherwise act like Go 1.10 and earlier to not break - // existing tests. - return false -} - func copyHeader(dst, src http.Header) { for k, vv := range src { for _, v := range vv { diff --git a/modules/caddyhttp/reverseproxy/streaming.go b/modules/caddyhttp/reverseproxy/streaming.go index 0c8e338..170afe8 100644 --- a/modules/caddyhttp/reverseproxy/streaming.go +++ b/modules/caddyhttp/reverseproxy/streaming.go @@ -13,8 +13,8 @@ // limitations under the License. // Most of the code in this file was initially borrowed from the Go -// standard library, which has this copyright notice: -// Copyright 2011 The Go Authors. +// standard library and modified; It had this copyright notice: +// Copyright 2011 The Go Authors package reverseproxy @@ -24,6 +24,8 @@ import ( "net/http" "sync" "time" + + "go.uber.org/zap" ) func (h Handler) handleUpgradeResponse(rw http.ResponseWriter, req *http.Request, res *http.Response) { @@ -97,19 +99,8 @@ func (h Handler) copyResponse(dst io.Writer, src io.Reader, flushInterval time.D } } - // TODO: Figure out how we want to do this... using custom buffer pool type seems unnecessary - // or maybe it is, depending on how we want to handle errors, - // see: https://github.com/golang/go/issues/21814 - // buf := bufPool.Get().(*bytes.Buffer) - // buf.Reset() - // defer bufPool.Put(buf) - // _, err := io.CopyBuffer(dst, src, ) - var buf []byte - // if h.BufferPool != nil { - // buf = h.BufferPool.Get() - // defer h.BufferPool.Put(buf) - // } - // // we could also see about a pool that returns values like this: make([]byte, 32*1024) + buf := streamingBufPool.Get().([]byte) + defer streamingBufPool.Put(buf) _, err := h.copyBuffer(dst, src, buf) return err } @@ -131,6 +122,7 @@ func (h Handler) copyBuffer(dst io.Writer, src io.Reader, buf []byte) (int64, er // something we need to report to the client, but read errors are a problem on our // end for sure. so we need to decide what we want.) // p.logf("copyBuffer: ReverseProxy read error during body copy: %v", rerr) + h.logger.Error("reading from backend", zap.Error(rerr)) } if nr > 0 { nw, werr := dst.Write(buf[:nr]) @@ -221,3 +213,9 @@ func (c switchProtocolCopier) copyToBackend(errc chan<- error) { _, err := io.Copy(c.backend, c.user) errc <- err } + +var streamingBufPool = sync.Pool{ + New: func() interface{} { + return make([]byte, 32*1024) + }, +} -- cgit v1.2.3