From: Ian Gudger Date: Fri, 18 May 2018 19:43:13 +0000 (-0700) Subject: net: fix DNS NXDOMAIN performance regression X-Git-Tag: go1.11beta1~335 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=11b3ee6fec40857af609674292362fb9a096dff6;p=gostls13.git net: fix DNS NXDOMAIN performance regression golang.org/cl/37879 unintentionally changed the way NXDOMAIN errors were handled. Before that change, resolution would fail on the first NXDOMAIN error and return to the user. After that change, the next server would be consulted and resolution would fail only after all servers had been consulted. This change restores the old behavior. Go 10.10.2: BenchmarkGoLookupIP-12 10000 174883 ns/op 11450 B/op 163 allocs/op BenchmarkGoLookupIPNoSuchHost-12 3000 670140 ns/op 52189 B/op 544 allocs/op BenchmarkGoLookupIPWithBrokenNameServer-12 1 5002568137 ns/op 163792 B/op 375 allocs/op before this change: BenchmarkGoLookupIP-12 10000 165501 ns/op 8585 B/op 94 allocs/op BenchmarkGoLookupIPNoSuchHost-12 1000 1204117 ns/op 83661 B/op 674 allocs/op BenchmarkGoLookupIPWithBrokenNameServer-12 1 5002629186 ns/op 159128 B/op 275 allocs/op after this change: BenchmarkGoLookupIP-12 10000 158102 ns/op 8585 B/op 94 allocs/op BenchmarkGoLookupIPNoSuchHost-12 2000 645364 ns/op 42990 B/op 356 allocs/op BenchmarkGoLookupIPWithBrokenNameServer-12 1 5002163437 ns/op 159144 B/op 275 allocs/op Fixes #25336 Change-Id: I315cd70330d1f66e54ce5a189a61c99f095bc138 Reviewed-on: https://go-review.googlesource.com/113815 Reviewed-by: Brad Fitzpatrick Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot --- diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go index 835957a37c..fe00fe19fe 100644 --- a/src/net/dnsclient_unix.go +++ b/src/net/dnsclient_unix.go @@ -157,7 +157,8 @@ func (r *Resolver) exchange(ctx context.Context, server string, q dnsmessage.Que return dnsmessage.Parser{}, dnsmessage.Header{}, errors.New("no answer from DNS server") } -func checkHeaders(p *dnsmessage.Parser, h dnsmessage.Header, name, server string) error { +// checkHeader performs basic sanity checks on the header. +func checkHeader(p *dnsmessage.Parser, h dnsmessage.Header, name, server string) error { _, err := p.AnswerHeader() if err != nil && err != dnsmessage.ErrSectionDone { return &DNSError{ @@ -173,14 +174,7 @@ func checkHeaders(p *dnsmessage.Parser, h dnsmessage.Header, name, server string return &DNSError{Err: "lame referral", Name: name, Server: server} } - // If answer errored for rcodes dnsRcodeSuccess or dnsRcodeNameError, - // it means the response in msg was not useful and trying another - // server probably won't help. Return now in those cases. - // TODO: indicate this in a more obvious way, such as a field on DNSError? - if h.RCode == dnsmessage.RCodeNameError { - return &DNSError{Err: errNoSuchHost.Error(), Name: name, Server: server} - } - if h.RCode != dnsmessage.RCodeSuccess { + if h.RCode != dnsmessage.RCodeSuccess && h.RCode != dnsmessage.RCodeNameError { // None of the error codes make sense // for the query we sent. If we didn't get // a name error and we didn't get success, @@ -265,7 +259,14 @@ func (r *Resolver) tryOneName(ctx context.Context, cfg *dnsConfig, name string, continue } - lastErr = checkHeaders(&p, h, name, server) + // The name does not exist, so trying another server won't help. + // + // TODO: indicate this in a more obvious way, such as a field on DNSError? + if h.RCode == dnsmessage.RCodeNameError { + return dnsmessage.Parser{}, "", &DNSError{Err: errNoSuchHost.Error(), Name: name, Server: server} + } + + lastErr = checkHeader(&p, h, name, server) if lastErr != nil { continue } diff --git a/src/net/dnsclient_unix_test.go b/src/net/dnsclient_unix_test.go index 1d3b78284c..a95b2fe645 100644 --- a/src/net/dnsclient_unix_test.go +++ b/src/net/dnsclient_unix_test.go @@ -746,6 +746,7 @@ func TestIgnoreLameReferrals(t *testing.T) { func BenchmarkGoLookupIP(b *testing.B) { testHookUninstaller.Do(uninstallTestHooks) ctx := context.Background() + b.ReportAllocs() for i := 0; i < b.N; i++ { goResolver.LookupIPAddr(ctx, "www.example.com") @@ -755,6 +756,7 @@ func BenchmarkGoLookupIP(b *testing.B) { func BenchmarkGoLookupIPNoSuchHost(b *testing.B) { testHookUninstaller.Do(uninstallTestHooks) ctx := context.Background() + b.ReportAllocs() for i := 0; i < b.N; i++ { goResolver.LookupIPAddr(ctx, "some.nonexistent") @@ -778,6 +780,7 @@ func BenchmarkGoLookupIPWithBrokenNameServer(b *testing.B) { b.Fatal(err) } ctx := context.Background() + b.ReportAllocs() for i := 0; i < b.N; i++ { goResolver.LookupIPAddr(ctx, "www.example.com") @@ -1437,7 +1440,7 @@ func TestIssue8434(t *testing.T) { t.Fatal("SkipAllQuestions failed:", err) } - err = checkHeaders(&p, h, "golang.org", "foo:53") + err = checkHeader(&p, h, "golang.org", "foo:53") if err == nil { t.Fatal("expected an error") } @@ -1455,28 +1458,39 @@ func TestIssue8434(t *testing.T) { // Issue 12778: verify that NXDOMAIN without RA bit errors as // "no such host" and not "server misbehaving" +// +// Issue 25336: verify that NXDOMAIN errors fail fast. func TestIssue12778(t *testing.T) { - msg := dnsmessage.Message{ - Header: dnsmessage.Header{ - RCode: dnsmessage.RCodeNameError, - RecursionAvailable: false, + lookups := 0 + fake := fakeDNSServer{ + rh: func(n, _ string, q dnsmessage.Message, _ time.Time) (dnsmessage.Message, error) { + lookups++ + return dnsmessage.Message{ + Header: dnsmessage.Header{ + ID: q.ID, + Response: true, + RCode: dnsmessage.RCodeNameError, + RecursionAvailable: false, + }, + Questions: q.Questions, + }, nil }, } + r := Resolver{PreferGo: true, Dial: fake.DialContext} - b, err := msg.Pack() - if err != nil { - t.Fatal("Pack failed:", err) - } - var p dnsmessage.Parser - h, err := p.Start(b) - if err != nil { - t.Fatal("Start failed:", err) - } - if err := p.SkipAllQuestions(); err != nil { - t.Fatal("SkipAllQuestions failed:", err) + resolvConf.mu.RLock() + conf := resolvConf.dnsConfig + resolvConf.mu.RUnlock() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + _, _, err := r.tryOneName(ctx, conf, ".", dnsmessage.TypeALL) + + if lookups != 1 { + t.Errorf("got %d lookups, wanted 1", lookups) } - err = checkHeaders(&p, h, "golang.org", "foo:53") if err == nil { t.Fatal("expected an error") }