Fix read timeout and add default timeout values.

By setting the read deadline in streamReader.Read(), the deadline was
extended by the read timeout on each subsequent call. To avoid this, the
deadline is set in FCGIClient.Request(), before the first read occurs.

See #1094.

caddyhttp/fastcgi/fastcgi_test.go

@@ -332,9 +332,6 @@ func TestReadTimeout(t *testing.T) {
 		t.Fatalf("Unable to create listener for test: %v", err)
 	}
 	defer listener.Close()
-	go fcgi.Serve(listener, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		time.Sleep(time.Second * 1)
-	}))
 
 	network, address := parseAddress(listener.Addr().String())
 	handler := Handler{
@@ -354,6 +351,10 @@ func TestReadTimeout(t *testing.T) {
 	}
 	w := httptest.NewRecorder()
 
+	go fcgi.Serve(listener, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		time.Sleep(time.Millisecond * 130)
+	}))
+
 	_, err = handler.ServeHTTP(w, r)
 	if err == nil {
 		t.Error("Expected i/o timeout error but had none")

caddyhttp/fastcgi/fcgiclient.go

@@ -212,6 +212,21 @@ func (c *FCGIClient) Close() error {
 	return c.rwc.Close()
 }
 
+// setReadDeadline sets a read deadline on FCGIClient based on the configured
+// readTimeout. A zero value for readTimeout means no deadline will be set.
+func (c *FCGIClient) setReadDeadline() error {
+	if c.readTimeout > 0 {
+		conn, ok := c.rwc.(net.Conn)
+		if ok {
+			conn.SetReadDeadline(time.Now().Add(c.readTimeout))
+		} else {
+			return fmt.Errorf("Could not set Client ReadTimeout")
+		}
+	}
+
+	return nil
+}
+
 func (c *FCGIClient) writeRecord(recType uint8, content []byte) error {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
@@ -350,20 +365,10 @@ func (w *streamReader) Read(p []byte) (n int, err error) {
 
 	if len(p) > 0 {
 		if len(w.buf) == 0 {
-
 			// filter outputs for error log
 			for {
 				rec := &record{}
 				var buf []byte
-				if readTimeout := w.c.ReadTimeout(); readTimeout > 0 {
-					conn, ok := w.c.rwc.(net.Conn)
-					if ok {
-						conn.SetReadDeadline(time.Now().Add(readTimeout))
-					} else {
-						err = fmt.Errorf("Could not set Client ReadTimeout")
-						return
-					}
-				}
 				buf, err = rec.read(w.c.rwc)
 				if err == errInvalidHeaderVersion {
 					continue
@@ -441,6 +446,10 @@ func (c *FCGIClient) Request(p map[string]string, req io.Reader) (resp *http.Res
 	tp := textproto.NewReader(rb)
 	resp = new(http.Response)
 
+	if err = c.setReadDeadline(); err != nil {
+		return
+	}
+
 	// Parse the response headers.
 	mimeHeader, err := tp.ReadMIMEHeader()
 	if err != nil && err != io.EOF {

caddyhttp/fastcgi/setup.go

@@ -53,15 +53,13 @@ func fastcgiParse(c *caddy.Controller) ([]Rule, error) {
 	var rules []Rule
 
 	for c.Next() {
-		var rule Rule
-
 		args := c.RemainingArgs()
 
 		if len(args) < 2 || len(args) > 3 {
 			return rules, c.ArgErr()
 		}
 
-		rule.Path = args[0]
+		rule := Rule{Path: args[0], ReadTimeout: 60 * time.Second}
 		upstreams := []string{args[1]}
 
 		if len(args) == 3 {
@@ -72,7 +70,7 @@ func fastcgiParse(c *caddy.Controller) ([]Rule, error) {
 
 		var err error
 		var pool int
-		var timeout time.Duration
+		var connectTimeout = 60 * time.Second
 		var dialers []dialer
 		var poolSize = -1
 
@@ -133,7 +131,7 @@ func fastcgiParse(c *caddy.Controller) ([]Rule, error) {
 				if !c.NextArg() {
 					return rules, c.ArgErr()
 				}
-				timeout, err = time.ParseDuration(c.Val())
+				connectTimeout, err = time.ParseDuration(c.Val())
 				if err != nil {
 					return rules, err
 				}
@@ -152,9 +150,18 @@ func fastcgiParse(c *caddy.Controller) ([]Rule, error) {
 		for _, rawAddress := range upstreams {
 			network, address := parseAddress(rawAddress)
 			if poolSize >= 0 {
-				dialers = append(dialers, &persistentDialer{size: poolSize, network: network, address: address, timeout: timeout})
+				dialers = append(dialers, &persistentDialer{
+					size:    poolSize,
+					network: network,
+					address: address,
+					timeout: connectTimeout,
+				})
 			} else {
-				dialers = append(dialers, basicDialer{network: network, address: address, timeout: timeout})
+				dialers = append(dialers, basicDialer{
+					network: network,
+					address: address,
+					timeout: connectTimeout,
+				})
 			}
 		}
 

caddyhttp/fastcgi/setup_test.go

@@ -73,45 +73,49 @@ func TestFastcgiParse(t *testing.T) {
 
 		{`fastcgi /blog 127.0.0.1:9000 php`,
 			false, []Rule{{
-				Path:       "/blog",
-				Address:    "127.0.0.1:9000",
-				Ext:        ".php",
-				SplitPath:  ".php",
-				dialer:     &loadBalancingDialer{dialers: []dialer{basicDialer{network: "tcp", address: "127.0.0.1:9000"}}},
-				IndexFiles: []string{"index.php"},
+				Path:        "/blog",
+				Address:     "127.0.0.1:9000",
+				Ext:         ".php",
+				SplitPath:   ".php",
+				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: "tcp", address: "127.0.0.1:9000", timeout: 60 * time.Second}}},
+				IndexFiles:  []string{"index.php"},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi /blog 127.0.0.1:9000 php {
 			upstream 127.0.0.1:9001
 		}`,
 			false, []Rule{{
-				Path:       "/blog",
-				Address:    "127.0.0.1:9000,127.0.0.1:9001",
-				Ext:        ".php",
-				SplitPath:  ".php",
-				dialer:     &loadBalancingDialer{dialers: []dialer{basicDialer{network: "tcp", address: "127.0.0.1:9000"}, basicDialer{network: "tcp", address: "127.0.0.1:9001"}}},
-				IndexFiles: []string{"index.php"},
+				Path:        "/blog",
+				Address:     "127.0.0.1:9000,127.0.0.1:9001",
+				Ext:         ".php",
+				SplitPath:   ".php",
+				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: "tcp", address: "127.0.0.1:9000", timeout: 60 * time.Second}, basicDialer{network: "tcp", address: "127.0.0.1:9001", timeout: 60 * time.Second}}},
+				IndexFiles:  []string{"index.php"},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi /blog 127.0.0.1:9000 {
 			upstream 127.0.0.1:9001 
 		}`,
 			false, []Rule{{
-				Path:       "/blog",
-				Address:    "127.0.0.1:9000,127.0.0.1:9001",
-				Ext:        "",
-				SplitPath:  "",
-				dialer:     &loadBalancingDialer{dialers: []dialer{basicDialer{network: "tcp", address: "127.0.0.1:9000"}, basicDialer{network: "tcp", address: "127.0.0.1:9001"}}},
-				IndexFiles: []string{},
+				Path:        "/blog",
+				Address:     "127.0.0.1:9000,127.0.0.1:9001",
+				Ext:         "",
+				SplitPath:   "",
+				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: "tcp", address: "127.0.0.1:9000", timeout: 60 * time.Second}, basicDialer{network: "tcp", address: "127.0.0.1:9001", timeout: 60 * time.Second}}},
+				IndexFiles:  []string{},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi / ` + defaultAddress + ` {
 	              split .html
 	              }`,
 			false, []Rule{{
-				Path:       "/",
-				Address:    defaultAddress,
-				Ext:        "",
-				SplitPath:  ".html",
-				dialer:     &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address}}},
-				IndexFiles: []string{},
+				Path:        "/",
+				Address:     defaultAddress,
+				Ext:         "",
+				SplitPath:   ".html",
+				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address, timeout: 60 * time.Second}}},
+				IndexFiles:  []string{},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi / ` + defaultAddress + ` {
 	              split .html
@@ -122,54 +126,59 @@ func TestFastcgiParse(t *testing.T) {
 				Address:         "127.0.0.1:9001",
 				Ext:             "",
 				SplitPath:       ".html",
-				dialer:          &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address}}},
+				dialer:          &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address, timeout: 60 * time.Second}}},
 				IndexFiles:      []string{},
 				IgnoredSubPaths: []string{"/admin", "/user"},
+				ReadTimeout:     60 * time.Second,
 			}}},
 		{`fastcgi / ` + defaultAddress + ` {
 	              pool 0
 	              }`,
 			false, []Rule{{
-				Path:       "/",
-				Address:    defaultAddress,
-				Ext:        "",
-				SplitPath:  "",
-				dialer:     &loadBalancingDialer{dialers: []dialer{&persistentDialer{size: 0, network: network, address: address}}},
-				IndexFiles: []string{},
+				Path:        "/",
+				Address:     defaultAddress,
+				Ext:         "",
+				SplitPath:   "",
+				dialer:      &loadBalancingDialer{dialers: []dialer{&persistentDialer{size: 0, network: network, address: address, timeout: 60 * time.Second}}},
+				IndexFiles:  []string{},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi / 127.0.0.1:8080  {
 			upstream 127.0.0.1:9000
 	              pool 5
 	              }`,
 			false, []Rule{{
-				Path:       "/",
-				Address:    "127.0.0.1:8080,127.0.0.1:9000",
-				Ext:        "",
-				SplitPath:  "",
-				dialer:     &loadBalancingDialer{dialers: []dialer{&persistentDialer{size: 5, network: "tcp", address: "127.0.0.1:8080"}, &persistentDialer{size: 5, network: "tcp", address: "127.0.0.1:9000"}}},
-				IndexFiles: []string{},
+				Path:        "/",
+				Address:     "127.0.0.1:8080,127.0.0.1:9000",
+				Ext:         "",
+				SplitPath:   "",
+				dialer:      &loadBalancingDialer{dialers: []dialer{&persistentDialer{size: 5, network: "tcp", address: "127.0.0.1:8080", timeout: 60 * time.Second}, &persistentDialer{size: 5, network: "tcp", address: "127.0.0.1:9000", timeout: 60 * time.Second}}},
+				IndexFiles:  []string{},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi / ` + defaultAddress + ` {
 	              split .php
 	              }`,
 			false, []Rule{{
-				Path:       "/",
-				Address:    defaultAddress,
-				Ext:        "",
-				SplitPath:  ".php",
-				dialer:     &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address}}},
-				IndexFiles: []string{},
+				Path:        "/",
+				Address:     defaultAddress,
+				Ext:         "",
+				SplitPath:   ".php",
+				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address, timeout: 60 * time.Second}}},
+				IndexFiles:  []string{},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi / ` + defaultAddress + ` {
 	              connect_timeout 5s
 	              }`,
 			false, []Rule{{
-				Path:       "/",
-				Address:    defaultAddress,
-				Ext:        "",
-				SplitPath:  "",
-				dialer:     &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address, timeout: 5 * time.Second}}},
-				IndexFiles: []string{},
+				Path:        "/",
+				Address:     defaultAddress,
+				Ext:         "",
+				SplitPath:   "",
+				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address, timeout: 5 * time.Second}}},
+				IndexFiles:  []string{},
+				ReadTimeout: 60 * time.Second,
 			}}},
 		{`fastcgi / ` + defaultAddress + ` {
 	              read_timeout 5s
@@ -179,7 +188,7 @@ func TestFastcgiParse(t *testing.T) {
 				Address:     defaultAddress,
 				Ext:         "",
 				SplitPath:   "",
-				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address}}},
+				dialer:      &loadBalancingDialer{dialers: []dialer{basicDialer{network: network, address: address, timeout: 60 * time.Second}}},
 				IndexFiles:  []string{},
 				ReadTimeout: 5 * time.Second,
 			}}},