reverseproxy: Pool copy buffers (minor optimization)

2 files changed, 20 insertions, 48 deletions

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)
+	},
+}