[release-branch.go1.7] net: restore per-query timeout logic

The handling of "options timeout:n" is supposed to be per individual
DNS server exchange, not per Lookup call.

Fixes #16865.

Change-Id: I2304579b9169c1515292f142cb372af9d37ff7c1
Reviewed-on: https://go-review.googlesource.com/28057
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/28640

diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go
index d12944c208f87063b2bba5ccfce22f47d9745c0a..b5b6ffb1c5060ebb025f984b2e1b1b6c334369d0 100644 (file)
--- a/src/net/dnsclient_unix.go
+++ b/src/net/dnsclient_unix.go
@@ -141,7 +141,7 @@ func (d *Dialer) dialDNS(ctx context.Context, network, server string) (dnsConn,
 }
 
 // exchange sends a query on the connection and hopes for a response.
-func exchange(ctx context.Context, server, name string, qtype uint16) (*dnsMsg, error) {
+func exchange(ctx context.Context, server, name string, qtype uint16, timeout time.Duration) (*dnsMsg, error) {
        d := testHookDNSDialer()
        out := dnsMsg{
                dnsMsgHdr: dnsMsgHdr{
@@ -152,6 +152,12 @@ func exchange(ctx context.Context, server, name string, qtype uint16) (*dnsMsg,
                },
        }
        for _, network := range []string{"udp", "tcp"} {
+               // TODO(mdempsky): Refactor so defers from UDP-based
+               // exchanges happen before TCP-based exchange.
+
+               ctx, cancel := context.WithDeadline(ctx, time.Now().Add(timeout))
+               defer cancel()
+
                c, err := d.dialDNS(ctx, network, server)
                if err != nil {
                        return nil, err
@@ -180,17 +186,10 @@ func tryOneName(ctx context.Context, cfg *dnsConfig, name string, qtype uint16)
                return "", nil, &DNSError{Err: "no DNS servers", Name: name}
        }
 
-       deadline := time.Now().Add(cfg.timeout)
-       if old, ok := ctx.Deadline(); !ok || deadline.Before(old) {
-               var cancel context.CancelFunc
-               ctx, cancel = context.WithDeadline(ctx, deadline)
-               defer cancel()
-       }
-
        var lastErr error
        for i := 0; i < cfg.attempts; i++ {
                for _, server := range cfg.servers {
-                       msg, err := exchange(ctx, server, name, qtype)
+                       msg, err := exchange(ctx, server, name, qtype, cfg.timeout)
                        if err != nil {
                                lastErr = &DNSError{
                                        Err:    err.Error(),

diff --git a/src/net/dnsclient_unix_test.go b/src/net/dnsclient_unix_test.go
index c953c1efaf6688483ae7ac838a94619013865af4..6ebeeaeb8f48996ac39a67a9a90bb8d2953c44b9 100644 (file)
--- a/src/net/dnsclient_unix_test.go
+++ b/src/net/dnsclient_unix_test.go
@@ -40,9 +40,9 @@ func TestDNSTransportFallback(t *testing.T) {
        testenv.MustHaveExternalNetwork(t)
 
        for _, tt := range dnsTransportFallbackTests {
-               ctx, cancel := context.WithTimeout(context.Background(), time.Duration(tt.timeout)*time.Second)
+               ctx, cancel := context.WithCancel(context.Background())
                defer cancel()
-               msg, err := exchange(ctx, tt.server, tt.name, tt.qtype)
+               msg, err := exchange(ctx, tt.server, tt.name, tt.qtype, time.Second)
                if err != nil {
                        t.Error(err)
                        continue
@@ -82,9 +82,9 @@ func TestSpecialDomainName(t *testing.T) {
 
        server := "8.8.8.8:53"
        for _, tt := range specialDomainNameTests {
-               ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
+               ctx, cancel := context.WithCancel(context.Background())
                defer cancel()
-               msg, err := exchange(ctx, server, tt.name, tt.qtype)
+               msg, err := exchange(ctx, server, tt.name, tt.qtype, 3*time.Second)
                if err != nil {
                        t.Error(err)
                        continue
@@ -501,7 +501,7 @@ func TestErrorForOriginalNameWhenSearching(t *testing.T) {
        d := &fakeDNSDialer{}
        testHookDNSDialer = func() dnsDialer { return d }
 
-       d.rh = func(s string, q *dnsMsg) (*dnsMsg, error) {
+       d.rh = func(s string, q *dnsMsg, _ time.Time) (*dnsMsg, error) {
                r := &dnsMsg{
                        dnsMsgHdr: dnsMsgHdr{
                                id: q.id,
@@ -540,14 +540,15 @@ func TestIgnoreLameReferrals(t *testing.T) {
        }
        defer conf.teardown()
 
-       if err := conf.writeAndUpdate([]string{"nameserver 192.0.2.1", "nameserver 192.0.2.2"}); err != nil {
+       if err := conf.writeAndUpdate([]string{"nameserver 192.0.2.1", // the one that will give a lame referral
+               "nameserver 192.0.2.2"}); err != nil {
                t.Fatal(err)
        }
 
        d := &fakeDNSDialer{}
        testHookDNSDialer = func() dnsDialer { return d }
 
-       d.rh = func(s string, q *dnsMsg) (*dnsMsg, error) {
+       d.rh = func(s string, q *dnsMsg, _ time.Time) (*dnsMsg, error) {
                t.Log(s, q)
                r := &dnsMsg{
                        dnsMsgHdr: dnsMsgHdr{
@@ -634,28 +635,30 @@ func BenchmarkGoLookupIPWithBrokenNameServer(b *testing.B) {
 
 type fakeDNSDialer struct {
        // reply handler
-       rh func(s string, q *dnsMsg) (*dnsMsg, error)
+       rh func(s string, q *dnsMsg, t time.Time) (*dnsMsg, error)
 }
 
 func (f *fakeDNSDialer) dialDNS(_ context.Context, n, s string) (dnsConn, error) {
-       return &fakeDNSConn{f.rh, s}, nil
+       return &fakeDNSConn{f.rh, s, time.Time{}}, nil
 }
 
 type fakeDNSConn struct {
-       rh func(s string, q *dnsMsg) (*dnsMsg, error)
+       rh func(s string, q *dnsMsg, t time.Time) (*dnsMsg, error)
        s  string
+       t  time.Time
 }
 
 func (f *fakeDNSConn) Close() error {
        return nil
 }
 
-func (f *fakeDNSConn) SetDeadline(time.Time) error {
+func (f *fakeDNSConn) SetDeadline(t time.Time) error {
+       f.t = t
        return nil
 }
 
 func (f *fakeDNSConn) dnsRoundTrip(q *dnsMsg) (*dnsMsg, error) {
-       return f.rh(f.s, q)
+       return f.rh(f.s, q, f.t)
 }
 
 // UDP round-tripper algorithm should ignore invalid DNS responses (issue 13281).
@@ -725,3 +728,72 @@ func TestIgnoreDNSForgeries(t *testing.T) {
                t.Errorf("got address %v, want %v", got, TestAddr)
        }
 }
+
+// Issue 16865. If a name server times out, continue to the next.
+func TestRetryTimeout(t *testing.T) {
+       origTestHookDNSDialer := testHookDNSDialer
+       defer func() { testHookDNSDialer = origTestHookDNSDialer }()
+
+       conf, err := newResolvConfTest()
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer conf.teardown()
+
+       if err := conf.writeAndUpdate([]string{"nameserver 192.0.2.1", // the one that will timeout
+               "nameserver 192.0.2.2"}); err != nil {
+               t.Fatal(err)
+       }
+
+       d := &fakeDNSDialer{}
+       testHookDNSDialer = func() dnsDialer { return d }
+
+       var deadline0 time.Time
+
+       d.rh = func(s string, q *dnsMsg, deadline time.Time) (*dnsMsg, error) {
+               t.Log(s, q, deadline)
+
+               if deadline.IsZero() {
+                       t.Error("zero deadline")
+               }
+
+               if s == "192.0.2.1:53" {
+                       deadline0 = deadline
+                       time.Sleep(10 * time.Millisecond)
+                       return nil, errTimeout
+               }
+
+               if deadline == deadline0 {
+                       t.Error("deadline didn't change")
+               }
+
+               r := &dnsMsg{
+                       dnsMsgHdr: dnsMsgHdr{
+                               id:                  q.id,
+                               response:            true,
+                               recursion_available: true,
+                       },
+                       question: q.question,
+                       answer: []dnsRR{
+                               &dnsRR_CNAME{
+                                       Hdr: dnsRR_Header{
+                                               Name:   q.question[0].Name,
+                                               Rrtype: dnsTypeCNAME,
+                                               Class:  dnsClassINET,
+                                       },
+                                       Cname: "golang.org",
+                               },
+                       },
+               }
+               return r, nil
+       }
+
+       _, err = goLookupCNAME(context.Background(), "www.golang.org")
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       if deadline0.IsZero() {
+               t.Error("deadline0 still zero", deadline0)
+       }
+}