From 881b826fb59a25102fd14ae3b420639479f2d6bf Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Wed, 27 May 2020 11:42:19 -0600
Subject: [PATCH] reverseproxy: Pool copy buffers (minor optimization)
---
.../caddyhttp/reverseproxy/reverseproxy.go | 42 ++++---------------
modules/caddyhttp/reverseproxy/streaming.go | 28 ++++++-------
2 files changed, 21 insertions(+), 49 deletions(-)
diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index 507995f0e..06802a017 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))
- 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)
- }
res.Body.Close() // close now, instead of defer, to populate res.Trailer
+ if err != nil {
+ // 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
+ }
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 0c8e3380e..170afe8e9 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)
+ },
+}