Merge pull request #1274 from mholt/fastcgi-timeout-defaults

Fix read timeout and add default timeout values.

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