From bb8706890bc5bab01894f7faf382b15364635d29 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 29 Aug 2016 13:53:32 -0700 Subject: [PATCH] [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 TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick Reviewed-on: https://go-review.googlesource.com/28640 --- src/net/dnsclient_unix.go | 17 +++--- src/net/dnsclient_unix_test.go | 96 +++++++++++++++++++++++++++++----- 2 files changed, 92 insertions(+), 21 deletions(-) diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go index d12944c208..b5b6ffb1c5 100644 --- 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 c953c1efaf..6ebeeaeb8f 100644 --- 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) + } +} -- 2.48.1