Repository: caddyserver/caddy (language: go)

Issue:
reverseproxy: Add read_timeout/send_timeout options for HTTP transport
Per the doc of [reverse_proxy](https://caddyserver.com/docs/caddyfile/directives/reverse_proxy) directive, the timeout options available for transports are:
- HTTP: dial_timeout, tls_timeout
- FastCGI: dial_timeout, read_timeout, write_timeout (not documented, but found [in code](https://github.com/caddyserver/caddy/blob/937ec342010894f7a6239883b10f6a107ff82c9f/modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go#L68-L75))

For the same reason listed [here](https://github.com/caddyserver/caddy/issues/2113#issuecomment-408840525), the read_timeout (like Nginx's [proxy_read_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout)) and send_timeout (like Nginx's  [proxy_send_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout)) options are also very useful for HTTP transport.


Discussion / hints:
It is documented, see here:

- https://caddyserver.com/docs/json/apps/http/servers/routes/handle/reverse_proxy/transport/http/
- https://caddyserver.com/docs/json/apps/http/servers/routes/handle/reverse_proxy/transport/fastcgi/

But we haven't wired up those timeouts up yet in the Caddyfile adapter for the `fastcgi` transport yet, true.

The timeouts we have available to us for the `http` transport are driven by what's available in Go's stdlib https://golang.org/pkg/net/http/#Transport. We have no plans to override internal behaviour to provide more timeouts. It's non-trivial and risky, could have some unintended side-effects.

I'll mark this as a feature request for the `fastcgi` timeouts, because it's true, that could be wired up, and I see no reason not to.
Actually I realized the `http` transport has a few options missing from the Caddyfile as well:

- `dial_fallback_delay`
- `response_header_timeout`
- `expect_continue_timeout`
- `max_response_header_size`

So I'll wire those up as well.
Yes, Go's [http.Transport](https://golang.org/pkg/net/http/#Transport) provides no read/write timeouts.

After googling, I found these two solutions:
- [Use context.Context with time.Timer to control read timeout](https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/) 
- [Implement a dialer to set read and write timeouts](https://gist.github.com/c4milo/275abc6eccbfd88ad56ca7c77947883a)
Yeah, read/write timeouts in a transport require overriding the Dialer if I recall correctly. We haven't done this (yet) because, well, you're in control of your reverse_proxy backends (or at least, you should be -- most of the time) and it hasn't come up before.

What do you need them for?
While using Nginx, we usually leverage read/write timeouts to protect HTTP connections from being occupied too long by some SLOW requests or responses, when their corresponding upstream behaves badly.

In this way, before we take action, the total concurrent connections opened by Nginx (or Caddy) can be controlled effectively, without heavily affecting other upstreams (maybe more important) reversely proxied by the same Nginx (or Caddy) instance.
After digging into the docs, I think setting the [upstreams/max_requests](https://caddyserver.com/docs/json/apps/http/servers/routes/handle/reverse_proxy#upstreams/max_requests) option can address the connection number problem I mentioned above.

Maybe there are other use-cases of Nginx's [proxy_read_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) and [proxy_send_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout), looking forward to thoughts from other folks :-)

Fix patch:
diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go
index a5321d1bdca..a2c85f90523 100644
--- a/modules/caddyhttp/reverseproxy/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/caddyfile.go
@@ -875,6 +875,26 @@ func (h *HTTPTransport) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				}
 				h.WriteBufferSize = int(size)
 
+			case "read_timeout":
+				if !d.NextArg() {
+					return d.ArgErr()
+				}
+				timeout, err := caddy.ParseDuration(d.Val())
+				if err != nil {
+					return d.Errf("invalid read timeout duration '%s': %v", d.Val(), err)
+				}
+				h.ReadTimeout = caddy.Duration(timeout)
+
+			case "write_timeout":
+				if !d.NextArg() {
+					return d.ArgErr()
+				}
+				timeout, err := caddy.ParseDuration(d.Val())
+				if err != nil {
+					return d.Errf("invalid write timeout duration '%s': %v", d.Val(), err)
+				}
+				h.WriteTimeout = caddy.Duration(timeout)
+
 			case "max_response_header":
 				if !d.NextArg() {
 					return d.ArgErr()
diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go
index 7b573f81ad3..ef72b8869bb 100644
--- a/modules/caddyhttp/reverseproxy/httptransport.go
+++ b/modules/caddyhttp/reverseproxy/httptransport.go
@@ -30,6 +30,7 @@ import (
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/modules/caddytls"
+	"go.uber.org/zap"
 	"golang.org/x/net/http2"
 )
 
@@ -88,6 +89,12 @@ type HTTPTransport struct {
 	// The size of the read buffer in bytes. Default: `4KiB`.
 	ReadBufferSize int `json:"read_buffer_size,omitempty"`
 
+	// The maximum time to wait for next read from backend. Default: no timeout.
+	ReadTimeout caddy.Duration `json:"read_timeout,omitempty"`
+
+	// The maximum time to wait for next write to backend. Default: no timeout.
+	WriteTimeout caddy.Duration `json:"write_timeout,omitempty"`
+
 	// The versions of HTTP to support. As a special case, "h2c"
 	// can be specified to use H2C (HTTP/2 over Cleartext) to the
 	// upstream (this feature is experimental and subject to
@@ -147,7 +154,7 @@ func (h *HTTPTransport) Provision(ctx caddy.Context) error {
 }
 
 // NewTransport builds a standard-lib-compatible http.Transport value from h.
-func (h *HTTPTransport) NewTransport(ctx caddy.Context) (*http.Transport, error) {
+func (h *HTTPTransport) NewTransport(caddyCtx caddy.Context) (*http.Transport, error) {
 	// Set keep-alive defaults if it wasn't otherwise configured
 	if h.KeepAlive == nil {
 		h.KeepAlive = &KeepAlive{
@@ -194,6 +201,7 @@ func (h *HTTPTransport) NewTransport(ctx caddy.Context) (*http.Transport, error)
 				network = dialInfo.Network
 				address = dialInfo.Address
 			}
+
 			conn, err := dialer.DialContext(ctx, network, address)
 			if err != nil {
 				// identify this error as one that occurred during
@@ -201,6 +209,18 @@ func (h *HTTPTransport) NewTransport(ctx caddy.Context) (*http.Transport, error)
 				// decide whether to retry a request
 				return nil, DialError{err}
 			}
+
+			// if read/write timeouts are configured and this is a TCP connection, enforce the timeouts
+			// by wrapping the connection with our own type
+			if tcpConn, ok := conn.(*net.TCPConn); ok && (h.ReadTimeout > 0 || h.WriteTimeout > 0) {
+				conn = &tcpRWTimeoutConn{
+					TCPConn:      tcpConn,
+					readTimeout:  time.Duration(h.ReadTimeout),
+					writeTimeout: time.Duration(h.WriteTimeout),
+					logger:       caddyCtx.Logger(h),
+				}
+			}
+
 			return conn, nil
 		},
 		MaxConnsPerHost:        h.MaxConnsPerHost,
@@ -214,7 +234,7 @@ func (h *HTTPTransport) NewTransport(ctx caddy.Context) (*http.Transport, error)
 	if h.TLS != nil {
 		rt.TLSHandshakeTimeout = time.Duration(h.TLS.HandshakeTimeout)
 		var err error
-		rt.TLSClientConfig, err = h.TLS.MakeTLSClientConfig(ctx)
+		rt.TLSClientConfig, err = h.TLS.MakeTLSClientConfig(caddyCtx)
 		if err != nil {
 			return nil, fmt.Errorf("making TLS client config: %v", err)
 		}
@@ -510,6 +530,36 @@ type KeepAlive struct {
 	IdleConnTimeout caddy.Duration `json:"idle_timeout,omitempty"`
 }
 
+// tcpRWTimeoutConn enforces read/write timeouts for a TCP connection.
+// If it fails to set deadlines, the error is logged but does not abort
+// the read/write attempt (ignoring the error is consistent with what
+// the standard library does: https://github.com/golang/go/blob/c5da4fb7ac5cb7434b41fc9a1df3bee66c7f1a4d/src/net/http/server.go#L981-L986)
+type tcpRWTimeoutConn struct {
+	*net.TCPConn
+	readTimeout, writeTimeout time.Duration
+	logger                    *zap.Logger
+}
+
+func (c *tcpRWTimeoutConn) Read(b []byte) (int, error) {
+	if c.readTimeout > 0 {
+		err := c.TCPConn.SetReadDeadline(time.Now().Add(c.readTimeout))
+		if err != nil {
+			c.logger.Error("failed to set read deadline", zap.Error(err))
+		}
+	}
+	return c.TCPConn.Read(b)
+}
+
+func (c *tcpRWTimeoutConn) Write(b []byte) (int, error) {
+	if c.writeTimeout > 0 {
+		err := c.TCPConn.SetWriteDeadline(time.Now().Add(c.writeTimeout))
+		if err != nil {
+			c.logger.Error("failed to set write deadline", zap.Error(err))
+		}
+	}
+	return c.TCPConn.Write(b)
+}
+
 // decodeBase64DERCert base64-decodes, then DER-decodes, certStr.
 func decodeBase64DERCert(certStr string) (*x509.Certificate, error) {
 	// decode base64


Test patch: