From bb8706890bc5bab01894f7faf382b15364635d29 Mon Sep 17 00:00:00 2001

From: Matthew Dempsky <[email protected]>

Date: Mon, 29 Aug 2016 13:53:32 -0700

Subject: [PATCH 09/38] [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 <[email protected]>

TryBot-Result: Gobot Gobot <[email protected]>

Reviewed-by: Brad Fitzpatrick <[email protected]>

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 d12944c..b5b6ffb 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 c953c1e..6ebeeae 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.7.4