From b8722d9af3b09944ef905e80bcd0a84515af24f2 Mon Sep 17 00:00:00 2001 From: ericdreeves Date: Fri, 25 Nov 2016 10:30:51 -0600 Subject: [PATCH] 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 | 7 +- caddyhttp/fastcgi/fcgiclient.go | 29 +++++--- caddyhttp/fastcgi/setup.go | 21 ++++-- caddyhttp/fastcgi/setup_test.go | 109 ++++++++++++++++-------------- 4 files changed, 96 insertions(+), 70 deletions(-) diff --git a/caddyhttp/fastcgi/fastcgi_test.go b/caddyhttp/fastcgi/fastcgi_test.go index 2190bf27..b84a78c5 100644 --- a/caddyhttp/fastcgi/fastcgi_test.go +++ b/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") diff --git a/caddyhttp/fastcgi/fcgiclient.go b/caddyhttp/fastcgi/fcgiclient.go index 95d391d9..1160be6b 100644 --- a/caddyhttp/fastcgi/fcgiclient.go +++ b/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 { diff --git a/caddyhttp/fastcgi/setup.go b/caddyhttp/fastcgi/setup.go index f37bc831..5382ff67 100644 --- a/caddyhttp/fastcgi/setup.go +++ b/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, + }) } } diff --git a/caddyhttp/fastcgi/setup_test.go b/caddyhttp/fastcgi/setup_test.go index c5e0fd82..c5e1b681 100644 --- a/caddyhttp/fastcgi/setup_test.go +++ b/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, }}},